This patch fixes a memory leak bug reported by syzbot. Link to the bug is given at [1].
A local variable name is used to hold the copied user buffer string using strndup_user. strndup_user allocates memory using kmalloc_track_caller in memdup_user. This kmalloc allocation needs to be followed by a kfree.
This patch has been tested by a compile test.
[1] https://syzkaller.appspot.com/bug?id=ce692a3aa13e00e335e090be7846c6eb60ddff7...
Reported-by: syzbot+b2098bc44728a4efb3e9@syzkaller.appspotmail.com Signed-off-by: Bharath Vedartham linux.bhar@gmail.com --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index f45bfb2..9798f6d 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -342,6 +342,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) } kfree(dmabuf->name); dmabuf->name = name; + kfree(name);
out_unlock: mutex_unlock(&dmabuf->lock);
Hi Bharath,
Thanks for taking the time to try to fix this report.
However, this doesn't look right.
On Fri, 2019-08-16 at 23:30 +0530, Bharath Vedartham wrote:
This patch fixes a memory leak bug reported by syzbot. Link to the bug is given at [1].
A local variable name is used to hold the copied user buffer string using strndup_user. strndup_user allocates memory using kmalloc_track_caller in memdup_user. This kmalloc allocation needs to be followed by a kfree.
This patch has been tested by a compile test.
[1] https://syzkaller.appspot.com/bug?id=ce692a3aa13e00e335e090be7846c6eb60ddff7...
Reported-by: syzbot+b2098bc44728a4efb3e9@syzkaller.appspotmail.com Signed-off-by: Bharath Vedartham linux.bhar@gmail.com
drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index f45bfb2..9798f6d 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -342,6 +342,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) } kfree(dmabuf->name); dmabuf->name = name;
- kfree(name);
Just by looking at this, you can deduce something is not right. You are assigning "name" to dmabuf->name, but then releasing "name"!
So now, dmabuf->name has free memory, which will lead to user-after-free issues.
Note also, that this function doesn't look leaky since the previous "name" is freed, before setting a new one.
Maybe the syzbot report is some kind of false positive?
Also, I _strongly_ suggest that in the future you don't compile-test only these kind of not trivial fixes. Since you are touching a crucial part of the kernel here, you should really be testing properly.
Specially since syzbot produces a reproducer.
Consider compile test as something you do when your changes are only cosmetic, and you are completely and absolutely sure things will be OK.
Thanks. Ezequiel
On Fri, Aug 16, 2019 at 05:14:24PM -0300, Ezequiel Garcia wrote:
Hi Bharath,
Thanks for taking the time to try to fix this report.
However, this doesn't look right.
On Fri, 2019-08-16 at 23:30 +0530, Bharath Vedartham wrote:
This patch fixes a memory leak bug reported by syzbot. Link to the bug is given at [1].
A local variable name is used to hold the copied user buffer string using strndup_user. strndup_user allocates memory using kmalloc_track_caller in memdup_user. This kmalloc allocation needs to be followed by a kfree.
This patch has been tested by a compile test.
[1] https://syzkaller.appspot.com/bug?id=ce692a3aa13e00e335e090be7846c6eb60ddff7...
Reported-by: syzbot+b2098bc44728a4efb3e9@syzkaller.appspotmail.com Signed-off-by: Bharath Vedartham linux.bhar@gmail.com
drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index f45bfb2..9798f6d 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -342,6 +342,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) } kfree(dmabuf->name); dmabuf->name = name;
- kfree(name);
Just by looking at this, you can deduce something is not right. You are assigning "name" to dmabuf->name, but then releasing "name"!
So now, dmabuf->name has free memory, which will lead to user-after-free issues.
Note also, that this function doesn't look leaky since the previous "name" is freed, before setting a new one.
Maybe the syzbot report is some kind of false positive?
Also, I _strongly_ suggest that in the future you don't compile-test only these kind of not trivial fixes. Since you are touching a crucial part of the kernel here, you should really be testing properly.
Specially since syzbot produces a reproducer.
Consider compile test as something you do when your changes are only cosmetic, and you are completely and absolutely sure things will be OK.
Thanks. Ezequiel
Hi Ezequiel,
Thank you for taking the time to review this.
I made a mistake here and thank you for notifying me of it.
Thank you for your comments, I ll keep them in mind before sending patches to the kernel :)
Thank you Bharath
dri-devel@lists.freedesktop.org
-
Bharath Vedartham -
Ezequiel Garcia