From: Ezequiel Garcia <ezequiel@collabora.com>
To: Bharath Vedartham <linux.bhar@gmail.com>, sumit.semwal@linaro.org
Cc: linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] dma-buf: Fix memory leak in dma_buf_set_name
Date: Fri, 16 Aug 2019 17:14:24 -0300 [thread overview]
Message-ID: <43d7adfb9ae9d25fc7c6093d3119c62f742df2cb.camel@collabora.com> (raw)
In-Reply-To: <1565978422-9661-1-git-send-email-linux.bhar@gmail.com>
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
next prev parent reply other threads:[~2019-08-16 20:14 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-16 18:00 [PATCH] dma-buf: Fix memory leak in dma_buf_set_name Bharath Vedartham
2019-08-16 20:14 ` Ezequiel Garcia [this message]
2019-08-18 18:05 ` Bharath Vedartham
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=43d7adfb9ae9d25fc7c6093d3119c62f742df2cb.camel@collabora.com \
--to=ezequiel@collabora.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux.bhar@gmail.com \
--cc=sumit.semwal@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).