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=ce692a3aa13e00e335e090be7846c6eb60ddff7a
Reported-by: [email protected]
Signed-off-by: Bharath Vedartham <[email protected]>
---
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);
--
2.7.4
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=ce692a3aa13e00e335e090be7846c6eb60ddff7a
>
> Reported-by: [email protected]
> Signed-off-by: Bharath Vedartham <[email protected]>
> ---
> 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=ce692a3aa13e00e335e090be7846c6eb60ddff7a
> >
> > Reported-by: [email protected]
> > Signed-off-by: Bharath Vedartham <[email protected]>
> > ---
> > 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