On Fri, May 08, 2020 at 12:11:03PM +0530, Charan Teja Reddy wrote:
> The following race occurs while accessing the dmabuf object exported as
> file:
> P1 P2
> dma_buf_release() dmabuffs_dname()
> [say lsof reading /proc/<P1 pid>/fd/<num>]
>
> read dmabuf stored in dentry->d_fsdata
> Free the dmabuf object
> Start accessing the dmabuf structure
>
> In the above description, the dmabuf object freed in P1 is being
> accessed from P2 which is resulting into the use-after-free. Below is
> the dump stack reported.
>
> We are reading the dmabuf object stored in the dentry->d_fsdata but
> there is no binding between the dentry and the dmabuf which means that
> the dmabuf can be freed while it is being read from ->d_fsdata and
> inuse. Reviews on the patch V1 says that protecting the dmabuf inuse
> with an extra refcount is not a viable solution as the exported dmabuf
> is already under file's refcount and keeping the multiple refcounts on
> the same object coordinated is not possible.
>
> As we are reading the dmabuf in ->d_fsdata just to get the user passed
> name, we can directly store the name in d_fsdata thus can avoid the
> reading of dmabuf altogether.
>
> Call Trace:
> kasan_report+0x12/0x20
> __asan_report_load8_noabort+0x14/0x20
> dmabuffs_dname+0x4f4/0x560
> tomoyo_realpath_from_path+0x165/0x660
> tomoyo_get_realpath
> tomoyo_check_open_permission+0x2a3/0x3e0
> tomoyo_file_open
> tomoyo_file_open+0xa9/0xd0
> security_file_open+0x71/0x300
> do_dentry_open+0x37a/0x1380
> vfs_open+0xa0/0xd0
> path_openat+0x12ee/0x3490
> do_filp_open+0x192/0x260
> do_sys_openat2+0x5eb/0x7e0
> do_sys_open+0xf2/0x180
>
> Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls")
> Reported-by: syzbot+3643a18836bce555bff6(a)syzkaller.appspotmail.com
> Cc: <stable(a)vger.kernel.org> [5.3+]
> Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org>
> ---
>
> Changes in v2:
>
> - Pass the user passed name in ->d_fsdata instead of dmabuf
> - Improve the commit message
>
> Changes in v1: (https://patchwork.kernel.org/patch/11514063/)
>
> drivers/dma-buf/dma-buf.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 01ce125..0071f7d 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -25,6 +25,7 @@
> #include <linux/mm.h>
> #include <linux/mount.h>
> #include <linux/pseudo_fs.h>
> +#include <linux/dcache.h>
>
> #include <uapi/linux/dma-buf.h>
> #include <uapi/linux/magic.h>
> @@ -40,15 +41,13 @@ struct dma_buf_list {
>
> static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen)
> {
> - struct dma_buf *dmabuf;
> char name[DMA_BUF_NAME_LEN];
> size_t ret = 0;
>
> - dmabuf = dentry->d_fsdata;
> - dma_resv_lock(dmabuf->resv, NULL);
> - if (dmabuf->name)
> - ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN);
> - dma_resv_unlock(dmabuf->resv);
> + spin_lock(&dentry->d_lock);
Are you sure this lock always protects d_fsdata?
> + if (dentry->d_fsdata)
> + ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN);
> + spin_unlock(&dentry->d_lock);
>
> return dynamic_dname(dentry, buffer, buflen, "/%s:%s",
> dentry->d_name.name, ret > 0 ? name : "");
If the above check fails the name will be what? How could d_name.name
be valid but d_fsdata not be valid?
> @@ -80,12 +79,16 @@ static int dma_buf_fs_init_context(struct fs_context *fc)
> static int dma_buf_release(struct inode *inode, struct file *file)
> {
> struct dma_buf *dmabuf;
> + struct dentry *dentry = file->f_path.dentry;
>
> if (!is_dma_buf_file(file))
> return -EINVAL;
>
> dmabuf = file->private_data;
>
> + spin_lock(&dentry->d_lock);
> + dentry->d_fsdata = NULL;
> + spin_unlock(&dentry->d_lock);
> BUG_ON(dmabuf->vmapping_counter);
>
> /*
> @@ -343,6 +346,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf)
> }
> kfree(dmabuf->name);
> dmabuf->name = name;
> + dmabuf->file->f_path.dentry->d_fsdata = name;
You are just changing the use of d_fsdata from being a pointer to the
dmabuf to being a pointer to the name string? What's to keep that name
string around and not have the same reference counting issues that the
dmabuf structure itself has? Who frees that string memory?
thanks,
greg k-h
On Tue, May 12, 2020 at 10:43:18AM +0530, Charan Teja Kalla wrote:
> > Ok, but watch out, now you have 2 different reference counts for the
> > same structure. Keeping them coordinated is almost always an impossible
> > task so you need to only rely on one. If you can't use the file api,
> > just drop all of the reference counting logic in there and only use the
> > kref one.
>
> I feel that changing the refcount logic now to dma-buf objects involve
> changes in
>
> the core dma-buf framework. NO? Instead, how about passing the user passed
> name directly
>
> in the ->d_fsdata inplace of dmabuf object? Because we just need user passed
> name in the
>
> dmabuffs_dname(). With this we can avoid the need for extra refcount on
> dmabuf.
Odd formatting :(
> Posted patch-V2: https://lkml.org/lkml/2020/5/8/158
Please just post links to lore.kernel.org, we have no control over
lkml.org at all.
I'll go review that patch now...
greg k-h
On Mon, 11 May 2020 at 19:37, Oded Gabbay <oded.gabbay(a)gmail.com> wrote:
>
> On Mon, May 11, 2020 at 12:11 PM Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote:
> >
> > It's the default.
> Thanks for catching that.
>
> >
> > Also so much for "we're not going to tell the graphics people how to
> > review their code", dma_fence is a pretty core piece of gpu driver
> > infrastructure. And it's very much uapi relevant, including piles of
> > corresponding userspace protocols and libraries for how to pass these
> > around.
> >
> > Would be great if habanalabs would not use this (from a quick look
> > it's not needed at all), since open source the userspace and playing
> > by the usual rules isn't on the table. If that's not possible (because
> > it's actually using the uapi part of dma_fence to interact with gpu
> > drivers) then we have exactly what everyone promised we'd want to
> > avoid.
>
> We don't use the uapi parts, we currently only using the fencing and
> signaling ability of this module inside our kernel code. But maybe I
> didn't understand what you request. You want us *not* to use this
> well-written piece of kernel code because it is only used by graphics
> drivers ?
> I'm sorry but I don't get this argument, if this is indeed what you meant.
We would rather drivers using a feature that has requirements on
correct userspace implementations of the feature have a userspace that
is open source and auditable.
Fencing is tricky, cross-device fencing is really tricky, and having
the ability for a closed userspace component to mess up other people's
drivers, think i915 shared with closed habana userspace and shared
fences, decreases ability to debug things.
Ideally we wouldn't offer users known untested/broken scenarios, so
yes we'd prefer that drivers that intend to expose a userspace fencing
api around dma-fence would adhere to the rules of the gpu drivers.
I'm not say you have to drop using dma-fence, but if you move towards
cross-device stuff I believe other drivers would be correct in
refusing to interact with fences from here.
Dave.
It's the default.
Also so much for "we're not going to tell the graphics people how to
review their code", dma_fence is a pretty core piece of gpu driver
infrastructure. And it's very much uapi relevant, including piles of
corresponding userspace protocols and libraries for how to pass these
around.
Would be great if habanalabs would not use this (from a quick look
it's not needed at all), since open source the userspace and playing
by the usual rules isn't on the table. If that's not possible (because
it's actually using the uapi part of dma_fence to interact with gpu
drivers) then we have exactly what everyone promised we'd want to
avoid.
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
Cc: Olof Johansson <olof(a)lixom.net>
Cc: Oded Gabbay <oded.gabbay(a)gmail.com>
Cc: Sumit Semwal <sumit.semwal(a)linaro.org>
Cc: linux-media(a)vger.kernel.org
Cc: linaro-mm-sig(a)lists.linaro.org
---
drivers/misc/habanalabs/command_submission.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/misc/habanalabs/command_submission.c b/drivers/misc/habanalabs/command_submission.c
index 409276b6374d..cc3ce759b6c3 100644
--- a/drivers/misc/habanalabs/command_submission.c
+++ b/drivers/misc/habanalabs/command_submission.c
@@ -46,7 +46,6 @@ static const struct dma_fence_ops hl_fence_ops = {
.get_driver_name = hl_fence_get_driver_name,
.get_timeline_name = hl_fence_get_timeline_name,
.enable_signaling = hl_fence_enable_signaling,
- .wait = dma_fence_default_wait,
.release = hl_fence_release
};
--
2.26.2
On Wed, May 06, 2020 at 02:00:10PM +0530, Charan Teja Kalla wrote:
> Thank you Greg for the reply.
>
> On 5/5/2020 3:38 PM, Greg KH wrote:
> > On Tue, Apr 28, 2020 at 01:24:02PM +0530, Charan Teja Reddy wrote:
> > > The following race occurs while accessing the dmabuf object exported as
> > > file:
> > > P1 P2
> > > dma_buf_release() dmabuffs_dname()
> > > [say lsof reading /proc/<P1 pid>/fd/<num>]
> > >
> > > read dmabuf stored in dentry->fsdata
> > > Free the dmabuf object
> > > Start accessing the dmabuf structure
> > >
> > > In the above description, the dmabuf object freed in P1 is being
> > > accessed from P2 which is resulting into the use-after-free. Below is
> > > the dump stack reported.
> > >
> > > Call Trace:
> > > kasan_report+0x12/0x20
> > > __asan_report_load8_noabort+0x14/0x20
> > > dmabuffs_dname+0x4f4/0x560
> > > tomoyo_realpath_from_path+0x165/0x660
> > > tomoyo_get_realpath
> > > tomoyo_check_open_permission+0x2a3/0x3e0
> > > tomoyo_file_open
> > > tomoyo_file_open+0xa9/0xd0
> > > security_file_open+0x71/0x300
> > > do_dentry_open+0x37a/0x1380
> > > vfs_open+0xa0/0xd0
> > > path_openat+0x12ee/0x3490
> > > do_filp_open+0x192/0x260
> > > do_sys_openat2+0x5eb/0x7e0
> > > do_sys_open+0xf2/0x180
> > >
> > > Fixes: bb2bb90 ("dma-buf: add DMA_BUF_SET_NAME ioctls")
> > Nit, please read the documentation for how to do a Fixes: line properly,
> > you need more digits:
> > Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls")
>
>
> Will update the patch
>
>
> > > Reported-by:syzbot+3643a18836bce555bff6@syzkaller.appspotmail.com
> > > Signed-off-by: Charan Teja Reddy<charante(a)codeaurora.org>
> > Also, any reason you didn't include the other mailing lists that
> > get_maintainer.pl said to?
>
>
> Really sorry for not sending to complete list. Added now.
>
>
> > And finally, no cc: stable in the s-o-b area for an issue that needs to
> > be backported to older kernels?
>
>
> Will update the patch.
>
>
> >
> > > ---
> > > drivers/dma-buf/dma-buf.c | 25 +++++++++++++++++++++++--
> > > include/linux/dma-buf.h | 1 +
> > > 2 files changed, 24 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > > index 570c923..069d8f78 100644
> > > --- a/drivers/dma-buf/dma-buf.c
> > > +++ b/drivers/dma-buf/dma-buf.c
> > > @@ -25,6 +25,7 @@
> > > #include <linux/mm.h>
> > > #include <linux/mount.h>
> > > #include <linux/pseudo_fs.h>
> > > +#include <linux/dcache.h>
> > > #include <uapi/linux/dma-buf.h>
> > > #include <uapi/linux/magic.h>
> > > @@ -38,18 +39,34 @@ struct dma_buf_list {
> > > static struct dma_buf_list db_list;
> > > +static void dmabuf_dent_put(struct dma_buf *dmabuf)
> > > +{
> > > + if (atomic_dec_and_test(&dmabuf->dent_count)) {
> > > + kfree(dmabuf->name);
> > > + kfree(dmabuf);
> > > + }
> > Why not just use a kref instead of an open-coded atomic value?
>
>
> Kref approach looks cleaner. will update the patch accordingly.
>
>
> > > +}
> > > +
> > > static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen)
> > > {
> > > struct dma_buf *dmabuf;
> > > char name[DMA_BUF_NAME_LEN];
> > > size_t ret = 0;
> > > + spin_lock(&dentry->d_lock);
> > > dmabuf = dentry->d_fsdata;
> > > + if (!dmabuf || !atomic_add_unless(&dmabuf->dent_count, 1, 0)) {
> > > + spin_unlock(&dentry->d_lock);
> > > + goto out;
> > How can dmabuf not be valid here?
> >
> > And isn't there already a usecount for the buffer?
>
>
> dmabuf exported as file simply relies on that file's refcount, thus fput()
> releases the dmabuf.
>
> We are storing the dmabuf in the dentry->d_fsdata but there is no binding
> between the dentry and the dmabuf. So, flow will be like
>
> 1) P1 calls fput(dmabuf_fd)
>
> 2) P2 trying to access the file information of P1.
> Eg: lsof command trying to list out the dmabuf_fd information using
> /proc/<P1 pid>/fd/dmabuf_fd
>
> 3) P1 calls the file->f_op->release(dmabuf_fd_file)(ends up in calling
> dma_buf_release()), thus frees up the dmabuf buffer.
>
> 4) P2 access the dmabuf stored in the dentry->d_fsdata which was freed in
> step 3.
>
> So we need to have some refcount mechanism to avoid the use-after-free in
> step 4.
Ok, but watch out, now you have 2 different reference counts for the
same structure. Keeping them coordinated is almost always an impossible
task so you need to only rely on one. If you can't use the file api,
just drop all of the reference counting logic in there and only use the
kref one.
good luck!
greg k-h
From: Daniel Vetter <daniel.vetter(a)intel.com>
commit a5bff92eaac45bdf6221badf9505c26792fdf99e upstream.
The uapi is the same on 32 and 64 bit, but the number isn't. Everyone
who botched this please re-read:
https://www.kernel.org/doc/html/v5.4-preprc-cpu/ioctl/botching-up-ioctls.ht…
Also, the type argument for the ioctl macros is for the type the void
__user *arg pointer points at, which in this case would be the
variable-sized char[] of a 0 terminated string. So this was botched in
more than just the usual ways.
Cc: Sumit Semwal <sumit.semwal(a)linaro.org>
Cc: Chenbo Feng <fengc(a)google.com>
Cc: Greg Hackmann <ghackmann(a)google.com>
Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch>
Cc: linux-media(a)vger.kernel.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: minchan(a)kernel.org
Cc: surenb(a)google.com
Cc: jenhaochen(a)google.com
Cc: Martin Liu <liumartin(a)google.com>
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
Tested-by: Martin Liu <liumartin(a)google.com>
Reviewed-by: Martin Liu <liumartin(a)google.com>
Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org>
[sumits: updated some checkpatch fixes, corrected author email]
Link: https://patchwork.freedesktop.org/patch/msgid/20200407133002.3486387-1-dani…
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/dma-buf/dma-buf.c | 3 ++-
include/uapi/linux/dma-buf.h | 6 ++++++
2 files changed, 8 insertions(+), 1 deletion(-)
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -388,7 +388,8 @@ static long dma_buf_ioctl(struct file *f
return ret;
- case DMA_BUF_SET_NAME:
+ case DMA_BUF_SET_NAME_A:
+ case DMA_BUF_SET_NAME_B:
return dma_buf_set_name(dmabuf, (const char __user *)arg);
default:
--- a/include/uapi/linux/dma-buf.h
+++ b/include/uapi/linux/dma-buf.h
@@ -39,6 +39,12 @@ struct dma_buf_sync {
#define DMA_BUF_BASE 'b'
#define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync)
+
+/* 32/64bitness of this uapi was botched in android, there's no difference
+ * between them in actual uapi, they're just different numbers.
+ */
#define DMA_BUF_SET_NAME _IOW(DMA_BUF_BASE, 1, const char *)
+#define DMA_BUF_SET_NAME_A _IOW(DMA_BUF_BASE, 1, u32)
+#define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, u64)
#endif
From: Daniel Vetter <daniel.vetter(a)intel.com>
commit a5bff92eaac45bdf6221badf9505c26792fdf99e upstream.
The uapi is the same on 32 and 64 bit, but the number isn't. Everyone
who botched this please re-read:
https://www.kernel.org/doc/html/v5.4-preprc-cpu/ioctl/botching-up-ioctls.ht…
Also, the type argument for the ioctl macros is for the type the void
__user *arg pointer points at, which in this case would be the
variable-sized char[] of a 0 terminated string. So this was botched in
more than just the usual ways.
Cc: Sumit Semwal <sumit.semwal(a)linaro.org>
Cc: Chenbo Feng <fengc(a)google.com>
Cc: Greg Hackmann <ghackmann(a)google.com>
Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch>
Cc: linux-media(a)vger.kernel.org
Cc: linaro-mm-sig(a)lists.linaro.org
Cc: minchan(a)kernel.org
Cc: surenb(a)google.com
Cc: jenhaochen(a)google.com
Cc: Martin Liu <liumartin(a)google.com>
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
Tested-by: Martin Liu <liumartin(a)google.com>
Reviewed-by: Martin Liu <liumartin(a)google.com>
Signed-off-by: Sumit Semwal <sumit.semwal(a)linaro.org>
[sumits: updated some checkpatch fixes, corrected author email]
Link: https://patchwork.freedesktop.org/patch/msgid/20200407133002.3486387-1-dani…
Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
---
drivers/dma-buf/dma-buf.c | 3 ++-
include/uapi/linux/dma-buf.h | 6 ++++++
2 files changed, 8 insertions(+), 1 deletion(-)
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -388,7 +388,8 @@ static long dma_buf_ioctl(struct file *f
return ret;
- case DMA_BUF_SET_NAME:
+ case DMA_BUF_SET_NAME_A:
+ case DMA_BUF_SET_NAME_B:
return dma_buf_set_name(dmabuf, (const char __user *)arg);
default:
--- a/include/uapi/linux/dma-buf.h
+++ b/include/uapi/linux/dma-buf.h
@@ -39,6 +39,12 @@ struct dma_buf_sync {
#define DMA_BUF_BASE 'b'
#define DMA_BUF_IOCTL_SYNC _IOW(DMA_BUF_BASE, 0, struct dma_buf_sync)
+
+/* 32/64bitness of this uapi was botched in android, there's no difference
+ * between them in actual uapi, they're just different numbers.
+ */
#define DMA_BUF_SET_NAME _IOW(DMA_BUF_BASE, 1, const char *)
+#define DMA_BUF_SET_NAME_A _IOW(DMA_BUF_BASE, 1, u32)
+#define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, u64)
#endif