Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

1 participants
2905 discussions

[PATCH v3 00/25] DRM: fix struct sg_table nents vs. orig_nents misuse

by Marek Szyprowski

Dear All, During the Exynos DRM GEM rework and fixing the issues in the drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most drivers in DRM framework incorrectly use nents and orig_nents entries of the struct sg_table. In case of the most DMA-mapping implementations exchanging those two entries or using nents for all loops on the scatterlist is harmless, because they both have the same value. There exists however a DMA-mapping implementations, for which such incorrect usage breaks things. The nents returned by dma_map_sg() might be lower than the nents passed as its parameter and this is perfectly fine. DMA framework or IOMMU is allowed to join consecutive chunks while mapping if such operation is supported by the underlying HW (bus, bridge, IOMMU, etc). Example of the case where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages is described here [2] The DMA-mapping framework documentation [3] states that dma_map_sg() returns the numer of the created entries in the DMA address space. However the subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be called with the original number of entries passed to dma_map_sg. The common pattern in DRM drivers were to assign the dma_map_sg() return value to sg_table->nents and use that value for the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also the code iterated over nents times to access the pages stored in the processed scatterlist, while it should use orig_nents as the numer of the page entries. I've tried to identify all such incorrect usage of sg_table->nents and this is a result of my research. It looks that the incorrect pattern has been copied over the many drivers mainly in the DRM subsystem. Too bad in most cases it even worked correctly if the system used simple, linear DMA-mapping implementation, for which swapping nents and orig_nents doesn't make any difference. To avoid similar issues in the future, I've introduced a common dma-mapping wrappers, which operate directly on the sg_table objects. The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix it fully. The driver creatively uses sg_table->orig_nents to store the size of the allocate scatterlist and ignores the number of the entries returned by dma_map_sg function. In this patchset I only fixed the sg_table objects exported by dmabuf related functions. I hope that I didn't break anything there. Patches are based on top of Linux next-20200504. Best regards, Marek Szyprowski References: [1] https://lkml.org/lkml/2020/3/27/555 [2] https://lkml.org/lkml/2020/3/29/65 [3] Documentation/DMA-API-HOWTO.txt Changelog: v3: - introduce dma_*_sgtable_* wrappers and use them in all patches v2: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - dropped most of the changes to drm/i915 - added fixes for rcar-du, xen, media and ion - fixed a few issues pointed by kbuild test robot - added wide cc: list for each patch v1: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@ar… - initial version Patch summary: Marek Szyprowski (25): dma-mapping: add generic helpers for mapping sgtable objects drm: core: fix common struct sg_table related issues drm: amdgpu: fix common struct sg_table related issues drm: armada: fix common struct sg_table related issues drm: etnaviv: fix common struct sg_table related issues drm: exynos: fix common struct sg_table related issues drm: i915: fix common struct sg_table related issues drm: lima: fix common struct sg_table related issues drm: msm: fix common struct sg_table related issues drm: panfrost: fix common struct sg_table related issues drm: radeon: fix common struct sg_table related issues drm: rockchip: fix common struct sg_table related issues drm: tegra: fix common struct sg_table related issues drm: virtio: fix common struct sg_table related issues drm: vmwgfx: fix common struct sg_table related issues xen: gntdev: fix common struct sg_table related issues drm: host1x: fix common struct sg_table related issues drm: rcar-du: fix common struct sg_table related issues dmabuf: fix common struct sg_table related issues staging: ion: fix common struct sg_table related issues staging: tegra-vde: fix common struct sg_table related issues misc: fastrpc: fix common struct sg_table related issues rapidio: fix common struct sg_table related issues samples: vfio-mdev/mbochs: fix common struct sg_table related issues media: pci: fix common ALSA DMA-mapping related codes drivers/dma-buf/heaps/heap-helpers.c | 13 +++++----- drivers/dma-buf/udmabuf.c | 7 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 ++--- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 9 +++---- drivers/gpu/drm/armada/armada_gem.c | 10 ++++---- drivers/gpu/drm/drm_cache.c | 2 +- drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++---- drivers/gpu/drm/drm_prime.c | 13 +++++----- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 ++++----- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 9 +++---- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 13 ++++------ drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +++--- drivers/gpu/drm/lima/lima_gem.c | 11 +++++--- drivers/gpu/drm/msm/msm_gem.c | 13 ++++------ drivers/gpu/drm/msm/msm_iommu.c | 2 +- drivers/gpu/drm/panfrost/panfrost_gem.c | 4 +-- drivers/gpu/drm/panfrost/panfrost_mmu.c | 5 ++-- drivers/gpu/drm/radeon/radeon_ttm.c | 11 ++++---- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 26 +++++++++---------- drivers/gpu/drm/tegra/gem.c | 27 ++++++++------------ drivers/gpu/drm/tegra/plane.c | 15 ++++------- drivers/gpu/drm/virtio/virtgpu_object.c | 17 ++++++------- drivers/gpu/drm/virtio/virtgpu_vq.c | 10 +++----- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 +++---------- drivers/gpu/host1x/job.c | 22 ++++++---------- drivers/media/pci/cx23885/cx23885-alsa.c | 2 +- drivers/media/pci/cx25821/cx25821-alsa.c | 2 +- drivers/media/pci/cx88/cx88-alsa.c | 2 +- drivers/media/pci/saa7134/saa7134-alsa.c | 2 +- drivers/media/platform/vsp1/vsp1_drm.c | 9 ++++--- drivers/misc/fastrpc.c | 4 +-- drivers/rapidio/devices/rio_mport_cdev.c | 8 +++--- drivers/staging/android/ion/ion.c | 25 ++++++++---------- drivers/staging/android/ion/ion_heap.c | 6 ++--- drivers/staging/android/ion/ion_system_heap.c | 2 +- drivers/staging/media/tegra-vde/iommu.c | 4 +-- drivers/xen/gntdev-dmabuf.c | 7 +++--- include/linux/dma-mapping.h | 32 ++++++++++++++++++++++++ include/linux/iommu.h | 6 +++++ samples/vfio-mdev/mbochs.c | 3 ++- 40 files changed, 202 insertions(+), 207 deletions(-) -- 1.9.1

5 years, 1 month

Re: [Linaro-mm-sig] [PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname

by Greg KH

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

5 years, 1 month

Re: [Linaro-mm-sig] [PATCH] dma-buf: fix use-after-free in dmabuffs_dname

by Greg KH

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

5 years, 1 month

Re: [Linaro-mm-sig] [Intel-gfx] [PATCH 3/3] misc/habalabs: don't set default fence_ops->wait

by Dave Airlie

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.

5 years, 1 month

[bug report] misc: fastrpc: Add support for context Invoke method

by Dan Carpenter

Hello Srinivas Kandagatla, The patch c68cfb718c8f: "misc: fastrpc: Add support for context Invoke method" from Feb 8, 2019, leads to the following static checker warning: drivers/misc/fastrpc.c:990 fastrpc_internal_invoke() warn: 'ctx->refcount.refcount.ref.counter' not decremented on lines: 990. drivers/misc/fastrpc.c 925 static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, 926 u32 handle, u32 sc, 927 struct fastrpc_invoke_args *args) 928 { 929 struct fastrpc_invoke_ctx *ctx = NULL; 930 int err = 0; 931 932 if (!fl->sctx) 933 return -EINVAL; 934 935 if (!fl->cctx->rpdev) 936 return -EPIPE; 937 938 ctx = fastrpc_context_alloc(fl, kernel, sc, args); refcount is 1. 939 if (IS_ERR(ctx)) 940 return PTR_ERR(ctx); 941 942 if (ctx->nscalars) { 943 err = fastrpc_get_args(kernel, ctx); 944 if (err) 945 goto bail; ^^^^^^^^^ Still holding one refcount. 946 } 947 948 /* make sure that all CPU memory writes are seen by DSP */ 949 dma_wmb(); 950 /* Send invoke buffer to remote dsp */ 951 err = fastrpc_invoke_send(fl->sctx, ctx, kernel, handle); ^^^^^^^^^^^^^^^^^^^ Takes a reference count. Refcount is now 2. 952 if (err) 953 goto bail; 954 955 if (kernel) { 956 if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) 957 err = -ETIMEDOUT; 958 } else { 959 err = wait_for_completion_interruptible(&ctx->work); This drops a refcount. 960 } 961 962 if (err) 963 goto bail; 964 965 /* Check the response from remote dsp */ 966 err = ctx->retval; 967 if (err) 968 goto bail; 969 970 if (ctx->nscalars) { 971 /* make sure that all memory writes by DSP are seen by CPU */ 972 dma_rmb(); 973 /* populate all the output buffers with results */ 974 err = fastrpc_put_args(ctx, kernel); 975 if (err) 976 goto bail; 977 } 978 979 bail: 980 if (err != -ERESTARTSYS && err != -ETIMEDOUT) { 981 /* We are done with this compute context */ 982 spin_lock(&fl->lock); 983 list_del(&ctx->node); 984 spin_unlock(&fl->lock); 985 fastrpc_context_put(ctx); If we're holding two refcounts then I think this leaks. 986 } 987 if (err) 988 dev_dbg(fl->sctx->dev, "Error: Invoke Failed %d\n", err); 989 990 return err; 991 } regards, dan carpenter

5 years, 1 month

[PATCH 2/3] dma-fence: use default wait function for mock fences

by Daniel Vetter

No need to micro-optmize when we're waiting in a mocked object ... Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.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/dma-buf/st-dma-fence.c | 41 ---------------------------------- 1 file changed, 41 deletions(-) diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index e593064341c8..8166d2984702 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -33,50 +33,9 @@ static void mock_fence_release(struct dma_fence *f) kmem_cache_free(slab_fences, to_mock_fence(f)); } -struct wait_cb { - struct dma_fence_cb cb; - struct task_struct *task; -}; - -static void mock_wakeup(struct dma_fence *f, struct dma_fence_cb *cb) -{ - wake_up_process(container_of(cb, struct wait_cb, cb)->task); -} - -static long mock_wait(struct dma_fence *f, bool intr, long timeout) -{ - const int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE; - struct wait_cb cb = { .task = current }; - - if (dma_fence_add_callback(f, &cb.cb, mock_wakeup)) - return timeout; - - while (timeout) { - set_current_state(state); - - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &f->flags)) - break; - - if (signal_pending_state(state, current)) - break; - - timeout = schedule_timeout(timeout); - } - __set_current_state(TASK_RUNNING); - - if (!dma_fence_remove_callback(f, &cb.cb)) - return timeout; - - if (signal_pending_state(state, current)) - return -ERESTARTSYS; - - return -ETIME; -} - static const struct dma_fence_ops mock_ops = { .get_driver_name = mock_name, .get_timeline_name = mock_name, - .wait = mock_wait, .release = mock_fence_release, }; -- 2.26.2

5 years, 1 month

[PATCH 3/3] misc/habalabs: don't set default fence_ops->wait

by Daniel Vetter

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

5 years, 1 month

Re: [Linaro-mm-sig] [PATCH] dma-buf: heaps: Remove Unneeded variable "ret" in dma_heap_dma_buf_begin_cpu_access()

by Brian Starkey

Hi Jason, On Wed, May 06, 2020 at 02:18:51PM +0800, Jason Yan wrote: > Fix the following coccicheck warning: > > drivers/dma-buf/heaps/heap-helpers.c:203:5-8: Unneeded variable: "ret". > Return "0" on line 216 > > Signed-off-by: Jason Yan <yanaijie(a)huawei.com> LGTM. Reviewed-by: Brian Starkey <brian.starkey(a)arm.com> Thanks, -Brian > --- > drivers/dma-buf/heaps/heap-helpers.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/heaps/heap-helpers.c b/drivers/dma-buf/heaps/heap-helpers.c > index 9f964ca3f59c..c82872501ba2 100644 > --- a/drivers/dma-buf/heaps/heap-helpers.c > +++ b/drivers/dma-buf/heaps/heap-helpers.c > @@ -200,7 +200,6 @@ static int dma_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > { > struct heap_helper_buffer *buffer = dmabuf->priv; > struct dma_heaps_attachment *a; > - int ret = 0; > > mutex_lock(&buffer->lock); > > @@ -213,7 +212,7 @@ static int dma_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > } > mutex_unlock(&buffer->lock); > > - return ret; > + return 0; > } > > static int dma_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, > -- > 2.21.1 >

5 years, 1 month

Re: [Linaro-mm-sig] [PATCH] dma-buf: fix use-after-free in dmabuffs_dname

by Greg KH

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

5 years, 1 month

[PATCH 5.4 01/57] dma-buf: Fix SET_NAME ioctl uapi

by Greg Kroah-Hartman

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

5 years, 1 month

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig