| Message ID | 1565978422-9661-1-git-send-email-linux.bhar@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: Fix memory leak in dma_buf_set_name | expand |
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: 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=ce692a3aa13e00e335e090be7846c6eb60ddff7a > > > > 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
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);
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: 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(+)