Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

8 participants
2915 discussions

Re: [Linaro-mm-sig] [PATCH v8 0/5] drm: address potential UAF bugs with drm_master ptrs

by Daniel Vetter

On Wed, Jul 21, 2021 at 2:44 PM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> wrote: > On 21/7/21 6:29 pm, Daniel Vetter wrote: > > On Wed, Jul 21, 2021 at 6:12 AM Desmond Cheong Zhi Xi > > <desmondcheongzx(a)gmail.com> wrote: > >> On 21/7/21 2:24 am, Daniel Vetter wrote: > >>> On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote: > >>>> Hi, > >>>> > >>>> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this. > >>>> > >>>> Overall, this series addresses potential use-after-free errors when dereferencing pointers to struct drm_master. These were identified after one such bug was caught by Syzbot in drm_getunique(): > >>>> https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f8… > >>>> > >>>> The series is broken up into five patches: > >>>> > >>>> 1. Move a call to drm_is_current_master() out from a section locked by &dev->mode_config.mutex in drm_mode_getconnector(). This patch does not apply to stable. > >>>> > >>>> 2. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info(). > >>>> > >>>> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c. > >>>> > >>>> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes. > >>>> > >>>> 5. Identify areas in drm_lease.c where pointers to struct drm_master are dereferenced, and ensure that the master pointers are not freed during use. > >>>> > >>>> v7 -> v8: > >>>> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI. > >>>> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI. > >>>> > >>>> v6 -> v7: > >>>> - Modify code alignment as suggested by the intel-gfx CI. > >>>> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI. > >>>> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI. > >>>> > >>>> v5 -> v6: > >>>> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. > >>>> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter. > >>>> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov. > >>>> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI. > >>>> > >>>> v4 -> v5: > >>>> - Add a new patch to the series that moves the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. > >>>> - Additionally, added a missing semicolon to the patch, caught by the intel-gfx CI. > >>>> > >>>> v3 -> v4: > >>>> - Move the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. As suggested by Daniel Vetter. This avoids a circular lock lock dependency as reported here https://patchwork.freedesktop.org/patch/440406/ > >>>> - Inside drm_is_current_master, instead of grabbing &fpriv->master->dev->master_mutex, we grab &fpriv->minor->dev->master_mutex to avoid dereferencing a null ptr if fpriv->master is not set. > >>>> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter. > >>>> - Additionally, add a file_priv->master NULL check inside drm_file_get_master, and handle the NULL result accordingly in drm_lease.c. As suggested by Daniel Vetter. > >>>> > >>>> v2 -> v3: > >>>> - Move the definition of drm_is_current_master and the _locked version higher up in drm_auth.c to avoid needing a forward declaration of drm_is_current_master_locked. As suggested by Daniel Vetter. > >>>> - Instead of leaking drm_device.master_mutex into drm_lease.c to protect drm_master pointers, add a new drm_file_get_master() function that returns drm_file->master while increasing its reference count, to prevent drm_file->master from being freed. As suggested by Daniel Vetter. > >>>> > >>>> v1 -> v2: > >>>> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov. > >>> > >>> Apologies for the delay, I missed your series. Maybe just ping next time > >>> around there's silence. > >>> > >>> Looks all great, merged to drm-misc-next. Given how complex this was I'm > >>> vary of just pushing this to -fixes without some solid testing. > >>> > >> > >> Hi Daniel, > >> > >> Thanks for merging, more testing definitely sounds good to me. > >> > >>> One thing I noticed is that drm_is_current_master could just use the > >>> spinlock, since it's only doing a read access. Care to type up that patch? > >>> > >> > >> I thought about this too, but I'm not sure if that's the best solution. > >> > >> drm_is_current_master calls drm_lease_owner which then walks up the tree > >> of master lessors. The spinlock protects the master of the current drm > >> file, but subsequent lessors aren't protected without holding the > >> device's master mutex. > > > > But this isn't a fpriv->master pointer, but a master->lessor pointer. > > Which should never ever be able to change (we'd have tons of uaf bugs > > around drm_lease_owner otherwise). So I don't think there's anything > > that dev->master_lock protects here that fpriv->master_lookup_lock > > doesn't protect already? > > > > Or am I missing something? > > > The comment in the struct drm_master says it's protected by > > mode_config.idr_mutex, but that only applies to the idrs and lists I > > think. > > > > Ah you're right, I also completely forgot that lessees hold a reference > to their lessor so nothing will be freed as long as the spinlock is > held. I'll prepare that patch then, thanks for pointing it out. btw since we now looked at all this in detail, can you perhaps do a patch to update the kerneldoc for all the lease fields in struct drm_master? I think moving them to the inline style and then adding comments for each field how locking/lifetime rules work would be really good. Since right now it's all fresh from for us. -Daniel > >>> Also, do you plan to look into that idea we've discussed to flush pending > >>> access when we revoke a master or a lease? I think that would be really > >>> nice improvement here. > >>> -Daniel > >>> > >> > >> Yup, now that the potential UAFs are addressed (hopefully), I'll take a > >> closer look and propose a patch for this. > > > > Thanks a lot. > > -Daniel > > > >> > >> Best wishes, > >> Desmond > >> > >>>> > >>>> Desmond Cheong Zhi Xi (5): > >>>> drm: avoid circular locks in drm_mode_getconnector > >>>> drm: avoid blocking in drm_clients_info's rcu section > >>>> drm: add a locked version of drm_is_current_master > >>>> drm: serialize drm_file.master with a new spinlock > >>>> drm: protect drm_master pointers in drm_lease.c > >>>> > >>>> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++--------- > >>>> drivers/gpu/drm/drm_connector.c | 5 +- > >>>> drivers/gpu/drm/drm_debugfs.c | 3 +- > >>>> drivers/gpu/drm/drm_file.c | 1 + > >>>> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++------- > >>>> include/drm/drm_auth.h | 1 + > >>>> include/drm/drm_file.h | 18 +++++-- > >>>> 7 files changed, 152 insertions(+), 50 deletions(-) > >>>> > >>>> -- > >>>> 2.25.1 > >>>> > >>> > >> > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 11 months

Re: [Linaro-mm-sig] [PATCH v8 0/5] drm: address potential UAF bugs with drm_master ptrs

by Daniel Vetter

On Wed, Jul 21, 2021 at 6:12 AM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> wrote: > On 21/7/21 2:24 am, Daniel Vetter wrote: > > On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote: > >> Hi, > >> > >> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this. > >> > >> Overall, this series addresses potential use-after-free errors when dereferencing pointers to struct drm_master. These were identified after one such bug was caught by Syzbot in drm_getunique(): > >> https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f8… > >> > >> The series is broken up into five patches: > >> > >> 1. Move a call to drm_is_current_master() out from a section locked by &dev->mode_config.mutex in drm_mode_getconnector(). This patch does not apply to stable. > >> > >> 2. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info(). > >> > >> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c. > >> > >> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes. > >> > >> 5. Identify areas in drm_lease.c where pointers to struct drm_master are dereferenced, and ensure that the master pointers are not freed during use. > >> > >> v7 -> v8: > >> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI. > >> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI. > >> > >> v6 -> v7: > >> - Modify code alignment as suggested by the intel-gfx CI. > >> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI. > >> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI. > >> > >> v5 -> v6: > >> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. > >> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter. > >> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov. > >> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI. > >> > >> v4 -> v5: > >> - Add a new patch to the series that moves the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. > >> - Additionally, added a missing semicolon to the patch, caught by the intel-gfx CI. > >> > >> v3 -> v4: > >> - Move the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. As suggested by Daniel Vetter. This avoids a circular lock lock dependency as reported here https://patchwork.freedesktop.org/patch/440406/ > >> - Inside drm_is_current_master, instead of grabbing &fpriv->master->dev->master_mutex, we grab &fpriv->minor->dev->master_mutex to avoid dereferencing a null ptr if fpriv->master is not set. > >> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter. > >> - Additionally, add a file_priv->master NULL check inside drm_file_get_master, and handle the NULL result accordingly in drm_lease.c. As suggested by Daniel Vetter. > >> > >> v2 -> v3: > >> - Move the definition of drm_is_current_master and the _locked version higher up in drm_auth.c to avoid needing a forward declaration of drm_is_current_master_locked. As suggested by Daniel Vetter. > >> - Instead of leaking drm_device.master_mutex into drm_lease.c to protect drm_master pointers, add a new drm_file_get_master() function that returns drm_file->master while increasing its reference count, to prevent drm_file->master from being freed. As suggested by Daniel Vetter. > >> > >> v1 -> v2: > >> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov. > > > > Apologies for the delay, I missed your series. Maybe just ping next time > > around there's silence. > > > > Looks all great, merged to drm-misc-next. Given how complex this was I'm > > vary of just pushing this to -fixes without some solid testing. > > > > Hi Daniel, > > Thanks for merging, more testing definitely sounds good to me. > > > One thing I noticed is that drm_is_current_master could just use the > > spinlock, since it's only doing a read access. Care to type up that patch? > > > > I thought about this too, but I'm not sure if that's the best solution. > > drm_is_current_master calls drm_lease_owner which then walks up the tree > of master lessors. The spinlock protects the master of the current drm > file, but subsequent lessors aren't protected without holding the > device's master mutex. But this isn't a fpriv->master pointer, but a master->lessor pointer. Which should never ever be able to change (we'd have tons of uaf bugs around drm_lease_owner otherwise). So I don't think there's anything that dev->master_lock protects here that fpriv->master_lookup_lock doesn't protect already? Or am I missing something? The comment in the struct drm_master says it's protected by mode_config.idr_mutex, but that only applies to the idrs and lists I think. > > Also, do you plan to look into that idea we've discussed to flush pending > > access when we revoke a master or a lease? I think that would be really > > nice improvement here. > > -Daniel > > > > Yup, now that the potential UAFs are addressed (hopefully), I'll take a > closer look and propose a patch for this. Thanks a lot. -Daniel > > Best wishes, > Desmond > > >> > >> Desmond Cheong Zhi Xi (5): > >> drm: avoid circular locks in drm_mode_getconnector > >> drm: avoid blocking in drm_clients_info's rcu section > >> drm: add a locked version of drm_is_current_master > >> drm: serialize drm_file.master with a new spinlock > >> drm: protect drm_master pointers in drm_lease.c > >> > >> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++--------- > >> drivers/gpu/drm/drm_connector.c | 5 +- > >> drivers/gpu/drm/drm_debugfs.c | 3 +- > >> drivers/gpu/drm/drm_file.c | 1 + > >> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++------- > >> include/drm/drm_auth.h | 1 + > >> include/drm/drm_file.h | 18 +++++-- > >> 7 files changed, 152 insertions(+), 50 deletions(-) > >> > >> -- > >> 2.25.1 > >> > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 11 months

[PATCH v2] dma-buf: Delete the DMA-BUF attachment sysfs statistics

by Hridya Valsaraju

The DMA-BUF attachment statistics form a subset of the DMA-BUF sysfs statistics that recently merged to the drm-misc tree. They are not UABI yet since they have not merged to the upstream Linux kernel. Since there has been a reported a performance regression due to the overhead of sysfs directory creation/teardown during dma_buf_attach()/dma_buf_detach(), this patch deletes the DMA-BUF attachment statistics from sysfs. Fixes: bdb8d06dfefd (dmabuf: Add the capability to expose DMA-BUF stats in sysfs) Signed-off-by: Hridya Valsaraju <hridya(a)google.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- Changes in v2: Updated commit message to clarify that the sysfs files being removed have not yet merged to upstream Linux and are hence not ABI. Hi Christian, I have updated the commit message as per your suggestion. Please do take another look when you get a chance. Thanks, Hridya .../ABI/testing/sysfs-kernel-dmabuf-buffers | 28 ---- drivers/dma-buf/dma-buf-sysfs-stats.c | 140 +----------------- drivers/dma-buf/dma-buf-sysfs-stats.h | 27 ---- drivers/dma-buf/dma-buf.c | 16 -- include/linux/dma-buf.h | 17 --- 5 files changed, 4 insertions(+), 224 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers index a243984ed420..5d3bc997dc64 100644 --- a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers @@ -22,31 +22,3 @@ KernelVersion: v5.13 Contact: Hridya Valsaraju <hridya(a)google.com> Description: This file is read-only and specifies the size of the DMA-BUF in bytes. - -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments -Date: May 2021 -KernelVersion: v5.13 -Contact: Hridya Valsaraju <hridya(a)google.com> -Description: This directory will contain subdirectories representing every - attachment of the DMA-BUF. - -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid> -Date: May 2021 -KernelVersion: v5.13 -Contact: Hridya Valsaraju <hridya(a)google.com> -Description: This directory will contain information on the attached device - and the number of current distinct device mappings. - -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/device -Date: May 2021 -KernelVersion: v5.13 -Contact: Hridya Valsaraju <hridya(a)google.com> -Description: This file is read-only and is a symlink to the attached device's - sysfs entry. - -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/map_counter -Date: May 2021 -KernelVersion: v5.13 -Contact: Hridya Valsaraju <hridya(a)google.com> -Description: This file is read-only and contains a map_counter indicating the - number of distinct device mappings of the attachment. diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c index a2638e84199c..053baadcada9 100644 --- a/drivers/dma-buf/dma-buf-sysfs-stats.c +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c @@ -40,14 +40,11 @@ * * * ``/sys/kernel/dmabuf/buffers/<inode_number>/exporter_name`` * * ``/sys/kernel/dmabuf/buffers/<inode_number>/size`` - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/device`` - * * ``/sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/map_counter`` * - * The information in the interface can also be used to derive per-exporter and - * per-device usage statistics. The data from the interface can be gathered - * on error conditions or other important events to provide a snapshot of - * DMA-BUF usage. It can also be collected periodically by telemetry to monitor - * various metrics. + * The information in the interface can also be used to derive per-exporter + * statistics. The data from the interface can be gathered on error conditions + * or other important events to provide a snapshot of DMA-BUF usage. + * It can also be collected periodically by telemetry to monitor various metrics. * * Detailed documentation about the interface is present in * Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers. @@ -121,120 +118,6 @@ static struct kobj_type dma_buf_ktype = { .default_groups = dma_buf_stats_default_groups, }; -#define to_dma_buf_attach_entry_from_kobj(x) container_of(x, struct dma_buf_attach_sysfs_entry, kobj) - -struct dma_buf_attach_stats_attribute { - struct attribute attr; - ssize_t (*show)(struct dma_buf_attach_sysfs_entry *sysfs_entry, - struct dma_buf_attach_stats_attribute *attr, char *buf); -}; -#define to_dma_buf_attach_stats_attr(x) container_of(x, struct dma_buf_attach_stats_attribute, attr) - -static ssize_t dma_buf_attach_stats_attribute_show(struct kobject *kobj, - struct attribute *attr, - char *buf) -{ - struct dma_buf_attach_stats_attribute *attribute; - struct dma_buf_attach_sysfs_entry *sysfs_entry; - - attribute = to_dma_buf_attach_stats_attr(attr); - sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); - - if (!attribute->show) - return -EIO; - - return attribute->show(sysfs_entry, attribute, buf); -} - -static const struct sysfs_ops dma_buf_attach_stats_sysfs_ops = { - .show = dma_buf_attach_stats_attribute_show, -}; - -static ssize_t map_counter_show(struct dma_buf_attach_sysfs_entry *sysfs_entry, - struct dma_buf_attach_stats_attribute *attr, - char *buf) -{ - return sysfs_emit(buf, "%u\n", sysfs_entry->map_counter); -} - -static struct dma_buf_attach_stats_attribute map_counter_attribute = - __ATTR_RO(map_counter); - -static struct attribute *dma_buf_attach_stats_default_attrs[] = { - &map_counter_attribute.attr, - NULL, -}; -ATTRIBUTE_GROUPS(dma_buf_attach_stats_default); - -static void dma_buf_attach_sysfs_release(struct kobject *kobj) -{ - struct dma_buf_attach_sysfs_entry *sysfs_entry; - - sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); - kfree(sysfs_entry); -} - -static struct kobj_type dma_buf_attach_ktype = { - .sysfs_ops = &dma_buf_attach_stats_sysfs_ops, - .release = dma_buf_attach_sysfs_release, - .default_groups = dma_buf_attach_stats_default_groups, -}; - -void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) -{ - struct dma_buf_attach_sysfs_entry *sysfs_entry; - - sysfs_entry = attach->sysfs_entry; - if (!sysfs_entry) - return; - - sysfs_delete_link(&sysfs_entry->kobj, &attach->dev->kobj, "device"); - - kobject_del(&sysfs_entry->kobj); - kobject_put(&sysfs_entry->kobj); -} - -int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, - unsigned int uid) -{ - struct dma_buf_attach_sysfs_entry *sysfs_entry; - int ret; - struct dma_buf *dmabuf; - - if (!attach) - return -EINVAL; - - dmabuf = attach->dmabuf; - - sysfs_entry = kzalloc(sizeof(struct dma_buf_attach_sysfs_entry), - GFP_KERNEL); - if (!sysfs_entry) - return -ENOMEM; - - sysfs_entry->kobj.kset = dmabuf->sysfs_entry->attach_stats_kset; - - attach->sysfs_entry = sysfs_entry; - - ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_attach_ktype, - NULL, "%u", uid); - if (ret) - goto kobj_err; - - ret = sysfs_create_link(&sysfs_entry->kobj, &attach->dev->kobj, - "device"); - if (ret) - goto link_err; - - return 0; - -link_err: - kobject_del(&sysfs_entry->kobj); -kobj_err: - kobject_put(&sysfs_entry->kobj); - attach->sysfs_entry = NULL; - - return ret; -} void dma_buf_stats_teardown(struct dma_buf *dmabuf) { struct dma_buf_sysfs_entry *sysfs_entry; @@ -243,7 +126,6 @@ void dma_buf_stats_teardown(struct dma_buf *dmabuf) if (!sysfs_entry) return; - kset_unregister(sysfs_entry->attach_stats_kset); kobject_del(&sysfs_entry->kobj); kobject_put(&sysfs_entry->kobj); } @@ -290,7 +172,6 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) { struct dma_buf_sysfs_entry *sysfs_entry; int ret; - struct kset *attach_stats_kset; if (!dmabuf || !dmabuf->file) return -EINVAL; @@ -315,21 +196,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) if (ret) goto err_sysfs_dmabuf; - /* create the directory for attachment stats */ - attach_stats_kset = kset_create_and_add("attachments", - &dmabuf_sysfs_no_uevent_ops, - &sysfs_entry->kobj); - if (!attach_stats_kset) { - ret = -ENOMEM; - goto err_sysfs_attach; - } - - sysfs_entry->attach_stats_kset = attach_stats_kset; - return 0; -err_sysfs_attach: - kobject_del(&sysfs_entry->kobj); err_sysfs_dmabuf: kobject_put(&sysfs_entry->kobj); dmabuf->sysfs_entry = NULL; diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h index 5f4703249117..a49c6e2650cc 100644 --- a/drivers/dma-buf/dma-buf-sysfs-stats.h +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h @@ -14,23 +14,8 @@ int dma_buf_init_sysfs_statistics(void); void dma_buf_uninit_sysfs_statistics(void); int dma_buf_stats_setup(struct dma_buf *dmabuf); -int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, - unsigned int uid); -static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, - int delta) -{ - struct dma_buf_attach_sysfs_entry *entry = attach->sysfs_entry; - entry->map_counter += delta; -} void dma_buf_stats_teardown(struct dma_buf *dmabuf); -void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach); -static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) -{ - struct dma_buf_sysfs_entry *entry = dmabuf->sysfs_entry; - - return entry->attachment_uid++; -} #else static inline int dma_buf_init_sysfs_statistics(void) @@ -44,19 +29,7 @@ static inline int dma_buf_stats_setup(struct dma_buf *dmabuf) { return 0; } -static inline int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, - unsigned int uid) -{ - return 0; -} static inline void dma_buf_stats_teardown(struct dma_buf *dmabuf) {} -static inline void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) {} -static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, - int delta) {} -static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) -{ - return 0; -} #endif #endif // _DMA_BUF_SYSFS_STATS_H diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 510b42771974..b1a6db71c656 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -738,7 +738,6 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, { struct dma_buf_attachment *attach; int ret; - unsigned int attach_uid; if (WARN_ON(!dmabuf || !dev)) return ERR_PTR(-EINVAL); @@ -764,13 +763,8 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, } dma_resv_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments); - attach_uid = dma_buf_update_attach_uid(dmabuf); dma_resv_unlock(dmabuf->resv); - ret = dma_buf_attach_stats_setup(attach, attach_uid); - if (ret) - goto err_sysfs; - /* When either the importer or the exporter can't handle dynamic * mappings we cache the mapping here to avoid issues with the * reservation object lock. @@ -797,7 +791,6 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, dma_resv_unlock(attach->dmabuf->resv); attach->sgt = sgt; attach->dir = DMA_BIDIRECTIONAL; - dma_buf_update_attachment_map_count(attach, 1 /* delta */); } return attach; @@ -814,7 +807,6 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_unlock(attach->dmabuf->resv); -err_sysfs: dma_buf_detach(dmabuf, attach); return ERR_PTR(ret); } @@ -864,7 +856,6 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) dma_resv_lock(attach->dmabuf->resv, NULL); __unmap_dma_buf(attach, attach->sgt, attach->dir); - dma_buf_update_attachment_map_count(attach, -1 /* delta */); if (dma_buf_is_dynamic(attach->dmabuf)) { dmabuf->ops->unpin(attach); @@ -878,7 +869,6 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach); - dma_buf_attach_stats_teardown(attach); kfree(attach); } EXPORT_SYMBOL_GPL(dma_buf_detach); @@ -1020,10 +1010,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, } } #endif /* CONFIG_DMA_API_DEBUG */ - - if (!IS_ERR(sg_table)) - dma_buf_update_attachment_map_count(attach, 1 /* delta */); - return sg_table; } EXPORT_SYMBOL_GPL(dma_buf_map_attachment); @@ -1061,8 +1047,6 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (dma_buf_is_dynamic(attach->dmabuf) && !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) dma_buf_unpin(attach); - - dma_buf_update_attachment_map_count(attach, -1 /* delta */); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 2b814fde0d11..678b2006be78 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -444,15 +444,6 @@ struct dma_buf { struct dma_buf_sysfs_entry { struct kobject kobj; struct dma_buf *dmabuf; - - /** - * @sysfs_entry.attachment_uid: - * - * This is protected by the dma_resv_lock() on @resv and is - * incremented on each attach. - */ - unsigned int attachment_uid; - struct kset *attach_stats_kset; } *sysfs_entry; #endif }; @@ -504,7 +495,6 @@ struct dma_buf_attach_ops { * @importer_ops: importer operations for this attachment, if provided * dma_buf_map/unmap_attachment() must be called with the dma_resv lock held. * @importer_priv: importer specific attachment data. - * @sysfs_entry: For exposing information about this attachment in sysfs. * * This structure holds the attachment information between the dma_buf buffer * and its user device(s). The list contains one attachment struct per device @@ -525,13 +515,6 @@ struct dma_buf_attachment { const struct dma_buf_attach_ops *importer_ops; void *importer_priv; void *priv; -#ifdef CONFIG_DMABUF_SYSFS_STATS - /* for sysfs stats */ - struct dma_buf_attach_sysfs_entry { - struct kobject kobj; - unsigned int map_counter; - } *sysfs_entry; -#endif }; /** -- 2.32.0.93.g670b81a890-goog

3 years, 11 months

Re: [Linaro-mm-sig] [PATCH v8 0/5] drm: address potential UAF bugs with drm_master ptrs

by Daniel Vetter

On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote: > Hi, > > In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this. > > Overall, this series addresses potential use-after-free errors when dereferencing pointers to struct drm_master. These were identified after one such bug was caught by Syzbot in drm_getunique(): > https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f8… > > The series is broken up into five patches: > > 1. Move a call to drm_is_current_master() out from a section locked by &dev->mode_config.mutex in drm_mode_getconnector(). This patch does not apply to stable. > > 2. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info(). > > 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c. > > 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes. > > 5. Identify areas in drm_lease.c where pointers to struct drm_master are dereferenced, and ensure that the master pointers are not freed during use. > > v7 -> v8: > - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI. > - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI. > > v6 -> v7: > - Modify code alignment as suggested by the intel-gfx CI. > - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI. > - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI. > > v5 -> v6: > - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. > - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter. > - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov. > - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI. > > v4 -> v5: > - Add a new patch to the series that moves the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. > - Additionally, added a missing semicolon to the patch, caught by the intel-gfx CI. > > v3 -> v4: > - Move the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. As suggested by Daniel Vetter. This avoids a circular lock lock dependency as reported here https://patchwork.freedesktop.org/patch/440406/ > - Inside drm_is_current_master, instead of grabbing &fpriv->master->dev->master_mutex, we grab &fpriv->minor->dev->master_mutex to avoid dereferencing a null ptr if fpriv->master is not set. > - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter. > - Additionally, add a file_priv->master NULL check inside drm_file_get_master, and handle the NULL result accordingly in drm_lease.c. As suggested by Daniel Vetter. > > v2 -> v3: > - Move the definition of drm_is_current_master and the _locked version higher up in drm_auth.c to avoid needing a forward declaration of drm_is_current_master_locked. As suggested by Daniel Vetter. > - Instead of leaking drm_device.master_mutex into drm_lease.c to protect drm_master pointers, add a new drm_file_get_master() function that returns drm_file->master while increasing its reference count, to prevent drm_file->master from being freed. As suggested by Daniel Vetter. > > v1 -> v2: > - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov. Apologies for the delay, I missed your series. Maybe just ping next time around there's silence. Looks all great, merged to drm-misc-next. Given how complex this was I'm vary of just pushing this to -fixes without some solid testing. One thing I noticed is that drm_is_current_master could just use the spinlock, since it's only doing a read access. Care to type up that patch? Also, do you plan to look into that idea we've discussed to flush pending access when we revoke a master or a lease? I think that would be really nice improvement here. -Daniel > > Desmond Cheong Zhi Xi (5): > drm: avoid circular locks in drm_mode_getconnector > drm: avoid blocking in drm_clients_info's rcu section > drm: add a locked version of drm_is_current_master > drm: serialize drm_file.master with a new spinlock > drm: protect drm_master pointers in drm_lease.c > > drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++--------- > drivers/gpu/drm/drm_connector.c | 5 +- > drivers/gpu/drm/drm_debugfs.c | 3 +- > drivers/gpu/drm/drm_file.c | 1 + > drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++------- > include/drm/drm_auth.h | 1 + > include/drm/drm_file.h | 18 +++++-- > 7 files changed, 152 insertions(+), 50 deletions(-) > > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 11 months

[PATCH 00/11] drm/msm: drm scheduler conversion and cleanups

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Conversion to gpu_scheduler, and bonus removal of drm_gem_object_put_locked() Rob Clark (11): drm/msm: Docs and misc cleanup drm/msm: Small submitqueue creation cleanup drm/msm: drop drm_gem_object_put_locked() drm: Drop drm_gem_object_put_locked() drm/msm/submit: Simplify out-fence-fd handling drm/msm: Consolidate submit bo state drm/msm: Track "seqno" fences by idr drm/msm: Return ERR_PTR() from submit_create() drm/msm: Conversion to drm scheduler drm/msm: Drop struct_mutex in submit path drm/msm: Utilize gpu scheduler priorities drivers/gpu/drm/drm_gem.c | 22 -- drivers/gpu/drm/msm/Kconfig | 1 + drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 4 +- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 +- drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 +- drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 7 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 12 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 4 +- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 6 +- drivers/gpu/drm/msm/msm_drv.c | 30 +- drivers/gpu/drm/msm/msm_fence.c | 39 --- drivers/gpu/drm/msm/msm_fence.h | 2 - drivers/gpu/drm/msm/msm_gem.c | 91 +----- drivers/gpu/drm/msm/msm_gem.h | 37 ++- drivers/gpu/drm/msm/msm_gem_submit.c | 300 ++++++++++++-------- drivers/gpu/drm/msm/msm_gpu.c | 50 +--- drivers/gpu/drm/msm/msm_gpu.h | 41 ++- drivers/gpu/drm/msm/msm_ringbuffer.c | 70 ++++- drivers/gpu/drm/msm/msm_ringbuffer.h | 12 + drivers/gpu/drm/msm/msm_submitqueue.c | 49 +++- include/drm/drm_gem.h | 2 - include/uapi/drm/msm_drm.h | 10 +- 23 files changed, 440 insertions(+), 359 deletions(-) -- 2.31.1

3 years, 11 months

Re: [Linaro-mm-sig] [PATCH v2] dma_buf: remove dmabuf sysfs teardown before release

by Christian König

Am 20.07.21 um 12:31 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Dmabuf sysfs stat is used for dmabuf info track. > But these file maybe still in use after buffer released, > should clear it before buffer release. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> Reviewed and pushed to drm-misc-next. Thanks, Christian. > --- > drivers/dma-buf/dma-buf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index b1a6db71c656..63d32261b63f 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -76,12 +76,12 @@ static void dma_buf_release(struct dentry *dentry) > */ > BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active); > > + dma_buf_stats_teardown(dmabuf); > dmabuf->ops->release(dmabuf); > > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > dma_resv_fini(dmabuf->resv); > > - dma_buf_stats_teardown(dmabuf); > module_put(dmabuf->owner); > kfree(dmabuf->name); > kfree(dmabuf);

3 years, 11 months

Re: [Linaro-mm-sig] [PATCH] dma_buf: remove dmabuf sysfs teardown before release/detach

by Christian König

Am 19.07.21 um 07:19 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Dmabuf sysfs stat is used for dmabuf info track. > but these file maybe still use after buffer release/detach, > should clear it before buffer release/detach. Please rebase on current drm-misc-next. The attachment sysfs files have been removed in the meantime. Thanks, Christian. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 510b42771974..9fa4620bd4bb 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -76,12 +76,12 @@ static void dma_buf_release(struct dentry *dentry) > */ > BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active); > > + dma_buf_stats_teardown(dmabuf); > dmabuf->ops->release(dmabuf); > > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > dma_resv_fini(dmabuf->resv); > > - dma_buf_stats_teardown(dmabuf); > module_put(dmabuf->owner); > kfree(dmabuf->name); > kfree(dmabuf); > @@ -875,10 +875,11 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > dma_resv_lock(dmabuf->resv, NULL); > list_del(&attach->node); > dma_resv_unlock(dmabuf->resv); > + > + dma_buf_attach_stats_teardown(attach); > if (dmabuf->ops->detach) > dmabuf->ops->detach(dmabuf, attach); > > - dma_buf_attach_stats_teardown(attach); > kfree(attach); > } > EXPORT_SYMBOL_GPL(dma_buf_detach);

3 years, 11 months

Re: [Linaro-mm-sig] [PATCH] dma_buf: remove dmabuf sysfs teardown before release/detach

by Christian König

Hi Guangming, Am 19.07.21 um 07:19 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Dmabuf sysfs stat is used for dmabuf info track. > but these file maybe still use after buffer release/detach, > should clear it before buffer release/detach. In general looks correct to me, but Hridya already send out a patch to partially revert the attachment sysfs files since they caused a bunch of more problems for some users. Please wait for that to land in branch drm-misc-next and then rebase yours on top of it. I will give you a ping when that is done. Thanks, Christian. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 510b42771974..9fa4620bd4bb 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -76,12 +76,12 @@ static void dma_buf_release(struct dentry *dentry) > */ > BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active); > > + dma_buf_stats_teardown(dmabuf); > dmabuf->ops->release(dmabuf); > > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > dma_resv_fini(dmabuf->resv); > > - dma_buf_stats_teardown(dmabuf); > module_put(dmabuf->owner); > kfree(dmabuf->name); > kfree(dmabuf); > @@ -875,10 +875,11 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > dma_resv_lock(dmabuf->resv, NULL); > list_del(&attach->node); > dma_resv_unlock(dmabuf->resv); > + > + dma_buf_attach_stats_teardown(attach); > if (dmabuf->ops->detach) > dmabuf->ops->detach(dmabuf, attach); > > - dma_buf_attach_stats_teardown(attach); > kfree(attach); > } > EXPORT_SYMBOL_GPL(dma_buf_detach);

3 years, 11 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: support users to change dma_buf.name

by Christian König

Am 14.07.21 um 14:29 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > User space user can call DMA_BUF_SET_NAME to set dma_buf.name, > also add a kernel api for users to do same thing at kernel side. Well if you want to add a kernel API to set the name you also need to provide an user for this. Christian. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/dma-buf.c | 28 ++++++++++++++++++++++------ > include/linux/dma-buf.h | 1 + > 2 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..949af232c644 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -331,20 +331,20 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) > * purpose between different devices. > * > * @dmabuf: [in] dmabuf buffer that will be renamed. > - * @buf: [in] A piece of userspace memory that contains the name of > + * @buf: [in] A piece of memory that contains the name of > * the dma-buf. > * > * Returns 0 on success. If the dma-buf buffer is already attached to > * devices, return -EBUSY. > * > */ > -static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > +long dma_buf_set_name(struct dma_buf *dmabuf, const char *buf) > { > - char *name = strndup_user(buf, DMA_BUF_NAME_LEN); > + char *name = kstrndup(buf, DMA_BUF_NAME_LEN, GFP_KERNEL); > long ret = 0; > > - if (IS_ERR(name)) > - return PTR_ERR(name); > + if (!name) > + return -ENOMEM; > > dma_resv_lock(dmabuf->resv, NULL); > if (!list_empty(&dmabuf->attachments)) { > @@ -361,6 +361,22 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > dma_resv_unlock(dmabuf->resv); > return ret; > } > +EXPORT_SYMBOL_GPL(dma_buf_set_name); > + > +static long > +dma_buf_set_name_user(struct dma_buf *dmabuf, const char __user *buf) > +{ > + char *name = strndup_user(buf, DMA_BUF_NAME_LEN); > + long ret = 0; > + > + if (IS_ERR(name)) > + return PTR_ERR(name); > + > + ret = dma_buf_set_name(dmabuf, name); > + kfree(name); > + > + return ret; > +} > > static long dma_buf_ioctl(struct file *file, > unsigned int cmd, unsigned long arg) > @@ -403,7 +419,7 @@ static long dma_buf_ioctl(struct file *file, > > case DMA_BUF_SET_NAME_A: > case DMA_BUF_SET_NAME_B: > - return dma_buf_set_name(dmabuf, (const char __user *)arg); > + return dma_buf_set_name_user(dmabuf, (const char __user *)arg); > > default: > return -ENOTTY; > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index efdc56b9d95f..e6612ab59a59 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -507,4 +507,5 @@ int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, > unsigned long); > int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > +long dma_buf_set_name(struct dma_buf *dmabuf, const char *name); > #endif /* __DMA_BUF_H__ */

3 years, 11 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: add kernel count for dma_buf

by Christian König

Am 14.07.21 um 14:03 schrieb guangming.cao(a)mediatek.com: > From: Guangming.Cao <guangming.cao(a)mediatek.com> > > On Wed, 2021-07-14 at 12:43 +0200, Christian K鰊ig wrote: >> Am 14.07.21 um 11:44 schrieb guangming.cao(a)mediatek.com: >>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> >>> On Wed, 2021-07-14 at 10:46 +0200, Christian K鰊ig wrote: >>>> Am 14.07.21 um 09:11 schrieb guangming.cao(a)mediatek.com: >>>>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>>>> >>>>> Add a refcount for kernel to prevent UAF(Use After Free) issue. >>>> Well NAK on so many levels. >>>> >>>>> We can assume a case like below: >>>>> 1. kernel space alloc dma_buf(file count = 1) >>>>> 2. kernel use dma_buf to get fd(file count = 1) >>>>> 3. userspace use fd to do mapping (file count = 2) >>>> Creating an userspace mapping increases the reference count for >>>> the >>>> underlying file object. >>>> >>>> See the implementation of mmap_region(): >>>> ... >>>> vma->vm_file = get_file(file); >>>> error = call_mmap(file, vma); >>>> ... >>>> >>>> What can happen is the the underlying exporter redirects the mmap >>>> to >>>> a >>>> different file, e.g. TTM or GEM drivers do that all the time. >>>> >>>> But this is fine since then the VA mapping is independent of the >>>> DMA- >>>> buf. >>>> >>>>> 4. kernel call dma_buf_put (file count = 1) >>>>> 5. userpsace close buffer fd(file count = 0) >>>>> 6. at this time, buffer is released, but va is valid!! >>>>> So we still can read/write buffer via mmap va, >>>>> it maybe cause memory leak, or kernel exception. >>>>> And also, if we use "ls -ll" to watch corresponding >>>>> process >>>>> fd link info, it also will cause kernel exception. >>>>> >>>>> Another case: >>>>> Using dma_buf_fd to generate more than 1 fd, because >>>>> dma_buf_fd will not increase file count, thus, when >>>>> close >>>>> the second fd, it maybe occurs error. >>>> Each opened fd will increase the reference count so this is >>>> certainly >>>> not correct what you describe here. >>>> >>>> Regards, >>>> Christian. >>>> >>> Yes, mmap will increase file count by calling get_file, so step[2] >>> -> >>> step[3], file count increase 1. >>> >>> But, dma_buf_fd() will not increase file count. >>> function "dma_buf_fd(struct dma_buf *dmabuf, int flags)" just get >>> an >>> unused fd, via call "get_unused_fd_flags(flags)", and call >>> "fd_install(fd, dmabuf->file)", it will let associated "struct >>> file*" >>> in task's fdt->fd[fd] points to this dma_buf.file, not increase the >>> file count of dma_buf.file. >>> I think this is confusing, I can get more than 1 fds via >>> dma_buf_fd, >>> but they don't need to close it because they don't increase file >>> count. >>> >>> However, dma_buf_put() can decrease file count at kernel side >>> directly. >>> If somebody write a ko to put file count of dma_buf.file many >>> times, it >>> will cause buffer freed earlier than except. At last on Android, I >>> think this is a little bit dangerous. >> dma_buf_fd() takes the dma_buf pointer and converts it into a fd. So >> the >> reference is consumed. >> >> That's why users of this interface make sure to get a separate >> reference, see drm_gem_prime_handle_to_fd() for example: >> >> ... >> out_have_handle: >> ret = dma_buf_fd(dmabuf, flags); >> /* >> * We must _not_ remove the buffer from the handle cache since >> the >> newly >> * created dma buf is already linked in the global obj->dma_buf >> pointer, >> * and that is invariant as long as a userspace gem handle >> exists. >> * Closing the handle will clean out the cache anyway, so we >> don't >> leak. >> */ >> if (ret < 0) { >> goto fail_put_dmabuf; >> } else { >> *prime_fd = ret; >> ret = 0; >> } >> >> goto out; >> >> fail_put_dmabuf: >> dma_buf_put(dmabuf); >> out: >> ... >> >> You could submit a patch to improve the documentation and explicitly >> note on dma_buf_fd() that the reference is consumed, but all of this >> is >> working perfectly fine. >> >> Regards, >> Christian. >> > Thanks for your reply! > > Yes, drm works fine because it fully understand what dma-buf api will > do. Improve the documentation is really good idea to prevent this case. > > But, what I can't understand is, for kernel api exported to > corresponding users, we don't need to ensure all api is safe? Well the API is perfectly safe, it is just not what you are expecting. > And for general cases, dma-buf framework also need to prevent this > case, isn't it, it will make dma-buf framework more strong? What we could do is to move getting the reference into that function if all users of that function does that anyway. This would then be more defensive because new users of dma_buf_fd() can't forget to grab a reference. But this needs a complete audit of the kernel with all of the users of dma_buf_fd(). Regards, Christian. > > > BRs! > Guangming >>>>> Solution: >>>>> Add a kernel count for dma_buf, and make sure the file >>>>> count >>>>> of dma_buf.file hold by kernel is 1. >>>>> >>>>> Notes: For this solution, kref couldn't work because kernel ref >>>>> maybe added from 0, but kref don't allow it. >>>>> >>>>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> >>>>> --- >>>>> drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++---- >>>>> include/linux/dma-buf.h | 6 ++++-- >>>>> 2 files changed, 23 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma- >>>>> buf.c >>>>> index 511fe0d217a0..04ee92aac8b9 100644 >>>>> --- a/drivers/dma-buf/dma-buf.c >>>>> +++ b/drivers/dma-buf/dma-buf.c >>>>> @@ -62,6 +62,7 @@ static void dma_buf_release(struct dentry >>>>> *dentry) >>>>> if (unlikely(!dmabuf)) >>>>> return; >>>>> >>>>> + WARN_ON(atomic64_read(&dmabuf->kernel_ref)); >>>>> BUG_ON(dmabuf->vmapping_counter); >>>>> >>>>> /* >>>>> @@ -555,6 +556,7 @@ struct dma_buf *dma_buf_export(const struct >>>>> dma_buf_export_info *exp_info) >>>>> goto err_module; >>>>> } >>>>> >>>>> + atomic64_set(&dmabuf->kernel_ref, 1); >>>>> dmabuf->priv = exp_info->priv; >>>>> dmabuf->ops = exp_info->ops; >>>>> dmabuf->size = exp_info->size; >>>>> @@ -617,6 +619,9 @@ int dma_buf_fd(struct dma_buf *dmabuf, int >>>>> flags) >>>>> >>>>> fd_install(fd, dmabuf->file); >>>>> >>>>> + /* Add file cnt for each new fd */ >>>>> + get_file(dmabuf->file); >>>>> + >>>>> return fd; >>>>> } >>>>> EXPORT_SYMBOL_GPL(dma_buf_fd); >>>>> @@ -626,12 +631,13 @@ EXPORT_SYMBOL_GPL(dma_buf_fd); >>>>> * @fd: [in] fd associated with the struct dma_buf to >>>>> be >>>>> returned >>>>> * >>>>> * On success, returns the struct dma_buf associated with an >>>>> fd; >>>>> uses >>>>> - * file's refcounting done by fget to increase refcount. >>>>> returns >>>>> ERR_PTR >>>>> - * otherwise. >>>>> + * dmabuf's ref refcounting done by kref_get to increase >>>>> refcount. >>>>> + * Returns ERR_PTR otherwise. >>>>> */ >>>>> struct dma_buf *dma_buf_get(int fd) >>>>> { >>>>> struct file *file; >>>>> + struct dma_buf *dmabuf; >>>>> >>>>> file = fget(fd); >>>>> >>>>> @@ -643,7 +649,12 @@ struct dma_buf *dma_buf_get(int fd) >>>>> return ERR_PTR(-EINVAL); >>>>> } >>>>> >>>>> - return file->private_data; >>>>> + dmabuf = file->private_data; >>>>> + /* replace file count increase as ref increase for kernel >>>>> user >>>>> */ >>>>> + get_dma_buf(dmabuf); >>>>> + fput(file); >>>>> + >>>>> + return dmabuf; >>>>> } >>>>> EXPORT_SYMBOL_GPL(dma_buf_get); >>>>> >>>>> @@ -662,7 +673,11 @@ void dma_buf_put(struct dma_buf *dmabuf) >>>>> if (WARN_ON(!dmabuf || !dmabuf->file)) >>>>> return; >>>>> >>>>> - fput(dmabuf->file); >>>>> + if (WARN_ON(!atomic64_read(&dmabuf->kernel_ref))) >>>>> + return; >>>>> + >>>>> + if (!atomic64_dec_return(&dmabuf->kernel_ref)) >>>>> + fput(dmabuf->file); >>>>> } >>>>> EXPORT_SYMBOL_GPL(dma_buf_put); >>>>> >>>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >>>>> index efdc56b9d95f..bc790cb028eb 100644 >>>>> --- a/include/linux/dma-buf.h >>>>> +++ b/include/linux/dma-buf.h >>>>> @@ -308,6 +308,7 @@ struct dma_buf_ops { >>>>> struct dma_buf { >>>>> size_t size; >>>>> struct file *file; >>>>> + atomic64_t kernel_ref; >>>>> struct list_head attachments; >>>>> const struct dma_buf_ops *ops; >>>>> struct mutex lock; >>>>> @@ -436,7 +437,7 @@ struct dma_buf_export_info { >>>>> .owner = THIS_MODULE } >>>>> >>>>> /** >>>>> - * get_dma_buf - convenience wrapper for get_file. >>>>> + * get_dma_buf - increase a kernel ref of dma-buf >>>>> * @dmabuf: [in] pointer to dma_buf >>>>> * >>>>> * Increments the reference count on the dma-buf, needed in >>>>> case >>>>> of drivers >>>>> @@ -446,7 +447,8 @@ struct dma_buf_export_info { >>>>> */ >>>>> static inline void get_dma_buf(struct dma_buf *dmabuf) >>>>> { >>>>> - get_file(dmabuf->file); >>>>> + if (atomic64_inc_return(&dmabuf->kernel_ref) == 1) >>>>> + get_file(dmabuf->file); >>>>> } >>>>> >>>>> /** >>

3 years, 11 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig