Hi,
This series contains some improvements that Daniel Vetter proposed following a discussion on a recent series:
https://lore.kernel.org/lkml/[email protected]/
While preparing these patches, I also noticed some unprotected uses of drm_master in the vmwgfx driver that can be addressed by new functions from the previous series.
This series is thus broken up into three patches:
1. Switch from the outer drm_device.master_mutex to the inner drm_file.master_lookup_lock in drm_is_current_master.
2. Update the kerneldoc for lease fields in drm_master to clarify lifetime/locking rules.
3. Prevent potential use-after-free bugs by replacing calls to drm_master_get with drm_file_get_master in vmwgfx_surface.c.
Best wishes,
Desmond
Desmond Cheong Zhi Xi (3):
drm: use the lookup lock in drm_is_current_master
drm: clarify lifetime/locking for drm_master's lease fields
drm/vmwgfx: fix potential UAF in vmwgfx_surface.c
drivers/gpu/drm/drm_auth.c | 9 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 +-
include/drm/drm_auth.h | 62 ++++++++++++++++++++-----
3 files changed, 58 insertions(+), 17 deletions(-)
--
2.25.1
drm_file.master should be protected by either drm_device.master_mutex
or drm_file.master_lookup_lock when being dereferenced. However,
drm_master_get is called on unprotected file_priv->master pointers in
vmw_surface_define_ioctl and vmw_gb_surface_define_internal.
This is fixed by replacing drm_master_get with drm_file_get_master.
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 0eba47762bed..5d53a5f9d123 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -865,7 +865,7 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
user_srf->prime.base.shareable = false;
user_srf->prime.base.tfile = NULL;
if (drm_is_primary_client(file_priv))
- user_srf->master = drm_master_get(file_priv->master);
+ user_srf->master = drm_file_get_master(file_priv);
/**
* From this point, the generic resource management functions
@@ -1534,7 +1534,7 @@ vmw_gb_surface_define_internal(struct drm_device *dev,
user_srf = container_of(srf, struct vmw_user_surface, srf);
if (drm_is_primary_client(file_priv))
- user_srf->master = drm_master_get(file_priv->master);
+ user_srf->master = drm_file_get_master(file_priv);
res = &user_srf->srf.res;
--
2.25.1
In particular, we make it clear that &drm_device.mode_config.idr_mutex
protects the lease idr and list structures for drm_master. The lessor
field itself doesn't need to be protected as it doesn't change after
it's set in drm_lease_create.
Additionally, we add descriptions for the lifetime of lessors and
leases to make it easier to reason about them.
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
include/drm/drm_auth.h | 62 ++++++++++++++++++++++++++++++++++--------
1 file changed, 51 insertions(+), 11 deletions(-)
diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
index f99d3417f304..c978c85464fa 100644
--- a/include/drm/drm_auth.h
+++ b/include/drm/drm_auth.h
@@ -58,12 +58,6 @@ struct drm_lock_data {
* @refcount: Refcount for this master object.
* @dev: Link back to the DRM device
* @driver_priv: Pointer to driver-private information.
- * @lessor: Lease holder
- * @lessee_id: id for lessees. Owners always have id 0
- * @lessee_list: other lessees of the same master
- * @lessees: drm_masters leasing from this one
- * @leases: Objects leased to this drm_master.
- * @lessee_idr: All lessees under this owner (only used where lessor == NULL)
*
* Note that master structures are only relevant for the legacy/primary device
* nodes, hence there can only be one per device, not one per drm_minor.
@@ -88,17 +82,63 @@ struct drm_master {
struct idr magic_map;
void *driver_priv;
- /* Tree of display resource leases, each of which is a drm_master struct
- * All of these get activated simultaneously, so drm_device master points
- * at the top of the tree (for which lessor is NULL). Protected by
- * &drm_device.mode_config.idr_mutex.
+ /**
+ * @lessor:
+ *
+ * Lease holder. The lessor does not change once it's set in
+ * drm_lease_create(). Each lessee holds a reference to its lessor that
+ * it releases upon being destroyed in drm_lease_destroy().
+ *
+ * Display resource leases form a tree of &struct drm_master. All of
+ * these get activated simultaneously, so &drm_device.master
+ * points at the top of the tree (for which lessor is NULL).
*/
-
struct drm_master *lessor;
+
+ /**
+ * @lessee_id:
+ *
+ * ID for lessees. Owners always have ID 0. Protected by
+ * &drm_device.mode_config's &drm_mode_config.idr_mutex.
+ */
int lessee_id;
+
+ /**
+ * @lessee_list:
+ *
+ * List of lessees of the same master. Protected by
+ * &drm_device.mode_config's &drm_mode_config.idr_mutex.
+ */
struct list_head lessee_list;
+
+ /**
+ * @lessees:
+ *
+ * List of drm_masters leasing from this one. Protected by
+ * &drm_device.mode_config's &drm_mode_config.idr_mutex.
+ *
+ * This master cannot be destroyed unless this list is empty as lessors
+ * are referenced by all their lessees.
+ */
struct list_head lessees;
+
+ /**
+ * @leases:
+ *
+ * Objects leased to this drm_master. Protected by
+ * &drm_device.mode_config's &drm_mode_config.idr_mutex.
+ *
+ * Objects are leased all together in drm_lease_create(), and are
+ * removed all together when the lease is revoked.
+ */
struct idr leases;
+
+ /**
+ * @lessee_idr:
+ *
+ * All lessees under this owner (only used where lessor is NULL).
+ * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex.
+ */
struct idr lessee_idr;
/* private: */
#if IS_ENABLED(CONFIG_DRM_LEGACY)
--
2.25.1
On Thu, Jul 22, 2021 at 05:29:28PM +0800, Desmond Cheong Zhi Xi wrote:
> In particular, we make it clear that &drm_device.mode_config.idr_mutex
> protects the lease idr and list structures for drm_master. The lessor
> field itself doesn't need to be protected as it doesn't change after
> it's set in drm_lease_create.
>
> Additionally, we add descriptions for the lifetime of lessors and
> leases to make it easier to reason about them.
>
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> ---
> include/drm/drm_auth.h | 62 ++++++++++++++++++++++++++++++++++--------
> 1 file changed, 51 insertions(+), 11 deletions(-)
>
> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> index f99d3417f304..c978c85464fa 100644
> --- a/include/drm/drm_auth.h
> +++ b/include/drm/drm_auth.h
> @@ -58,12 +58,6 @@ struct drm_lock_data {
> * @refcount: Refcount for this master object.
> * @dev: Link back to the DRM device
> * @driver_priv: Pointer to driver-private information.
> - * @lessor: Lease holder
> - * @lessee_id: id for lessees. Owners always have id 0
> - * @lessee_list: other lessees of the same master
> - * @lessees: drm_masters leasing from this one
> - * @leases: Objects leased to this drm_master.
> - * @lessee_idr: All lessees under this owner (only used where lessor == NULL)
> *
> * Note that master structures are only relevant for the legacy/primary device
> * nodes, hence there can only be one per device, not one per drm_minor.
> @@ -88,17 +82,63 @@ struct drm_master {
> struct idr magic_map;
> void *driver_priv;
>
> - /* Tree of display resource leases, each of which is a drm_master struct
> - * All of these get activated simultaneously, so drm_device master points
> - * at the top of the tree (for which lessor is NULL). Protected by
> - * &drm_device.mode_config.idr_mutex.
> + /**
> + * @lessor:
> + *
> + * Lease holder. The lessor does not change once it's set in
Lessor is the lease grantor, lessee is the one receiving the lease. Maybe
clarify this with
"Lease grantor, only set if this struct drm_master represents a lessee
holding a lease of objects from @lessor. Full owners of the device have
this set to NULL."
> + * drm_lease_create(). Each lessee holds a reference to its lessor that
I also figured it'd be a good idea to link to the drm_lease docs here to
explain the concepts, but alas we don't have those :-( Hence at least
define what we mean with lessor, lessee, lease and owner.
> + * it releases upon being destroyed in drm_lease_destroy().
> + *
> + * Display resource leases form a tree of &struct drm_master. All of
For now we've limited the tree to a depth of 1, you can't create another
lease if yourself are a lease. I guess another reason to update the
drm_lease.c docs.
So maybe add "Currently the lease tree depth is limited to 1."
> + * these get activated simultaneously, so &drm_device.master
> + * points at the top of the tree (for which lessor is NULL).
> */
> -
> struct drm_master *lessor;
> +
> + /**
> + * @lessee_id:
> + *
> + * ID for lessees. Owners always have ID 0. Protected by
Maybe clarify to "Owners (i.e. @lessor is NULL) ..."
> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> + */
> int lessee_id;
> +
> + /**
> + * @lessee_list:
> + *
> + * List of lessees of the same master. Protected by
We try to distinguis between list entries and the list, even though it's
the same struct. So maybe
"List entry of lessees of @lessor, where they are linked to @lessee. Not
use for owners."
> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> + */
> struct list_head lessee_list;
> +
> + /**
> + * @lessees:
> + *
> + * List of drm_masters leasing from this one. Protected by
> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> + *
> + * This master cannot be destroyed unless this list is empty as lessors
> + * are referenced by all their lessees.
Maybe add "this list is empty of no leases have been granted."
> + */
> struct list_head lessees;
> +
> + /**
> + * @leases:
> + *
> + * Objects leased to this drm_master. Protected by
> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> + *
> + * Objects are leased all together in drm_lease_create(), and are
> + * removed all together when the lease is revoked.
> + */
> struct idr leases;
> +
> + /**
> + * @lessee_idr:
> + *
> + * All lessees under this owner (only used where lessor is NULL).
@lessor so it's highlighted correctly
> + * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex.
> + */
> struct idr lessee_idr;
> /* private: */
With the nits addressed:
Reviewed-by: Daniel Vetter <[email protected]>
Thanks for updating the docs!
-Daniel
> #if IS_ENABLED(CONFIG_DRM_LEGACY)
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Jul 22, 2021 at 05:29:29PM +0800, Desmond Cheong Zhi Xi wrote:
> drm_file.master should be protected by either drm_device.master_mutex
> or drm_file.master_lookup_lock when being dereferenced. However,
> drm_master_get is called on unprotected file_priv->master pointers in
> vmw_surface_define_ioctl and vmw_gb_surface_define_internal.
>
> This is fixed by replacing drm_master_get with drm_file_get_master.
>
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
Reviewed-by: Daniel Vetter <[email protected]>
I'll let Zack take a look at this and expect him to push this patch to
drm-misc.git.
-Daniel
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index 0eba47762bed..5d53a5f9d123 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> @@ -865,7 +865,7 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
> user_srf->prime.base.shareable = false;
> user_srf->prime.base.tfile = NULL;
> if (drm_is_primary_client(file_priv))
> - user_srf->master = drm_master_get(file_priv->master);
> + user_srf->master = drm_file_get_master(file_priv);
>
> /**
> * From this point, the generic resource management functions
> @@ -1534,7 +1534,7 @@ vmw_gb_surface_define_internal(struct drm_device *dev,
>
> user_srf = container_of(srf, struct vmw_user_surface, srf);
> if (drm_is_primary_client(file_priv))
> - user_srf->master = drm_master_get(file_priv->master);
> + user_srf->master = drm_file_get_master(file_priv);
>
> res = &user_srf->srf.res;
>
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 22/7/21 6:35 pm, Daniel Vetter wrote:
> On Thu, Jul 22, 2021 at 05:29:28PM +0800, Desmond Cheong Zhi Xi wrote:
>> In particular, we make it clear that &drm_device.mode_config.idr_mutex
>> protects the lease idr and list structures for drm_master. The lessor
>> field itself doesn't need to be protected as it doesn't change after
>> it's set in drm_lease_create.
>>
>> Additionally, we add descriptions for the lifetime of lessors and
>> leases to make it easier to reason about them.
>>
>> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
>> ---
>> include/drm/drm_auth.h | 62 ++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 51 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
>> index f99d3417f304..c978c85464fa 100644
>> --- a/include/drm/drm_auth.h
>> +++ b/include/drm/drm_auth.h
>> @@ -58,12 +58,6 @@ struct drm_lock_data {
>> * @refcount: Refcount for this master object.
>> * @dev: Link back to the DRM device
>> * @driver_priv: Pointer to driver-private information.
>> - * @lessor: Lease holder
>> - * @lessee_id: id for lessees. Owners always have id 0
>> - * @lessee_list: other lessees of the same master
>> - * @lessees: drm_masters leasing from this one
>> - * @leases: Objects leased to this drm_master.
>> - * @lessee_idr: All lessees under this owner (only used where lessor == NULL)
>> *
>> * Note that master structures are only relevant for the legacy/primary device
>> * nodes, hence there can only be one per device, not one per drm_minor.
>> @@ -88,17 +82,63 @@ struct drm_master {
>> struct idr magic_map;
>> void *driver_priv;
>>
>> - /* Tree of display resource leases, each of which is a drm_master struct
>> - * All of these get activated simultaneously, so drm_device master points
>> - * at the top of the tree (for which lessor is NULL). Protected by
>> - * &drm_device.mode_config.idr_mutex.
>> + /**
>> + * @lessor:
>> + *
>> + * Lease holder. The lessor does not change once it's set in
>
> Lessor is the lease grantor, lessee is the one receiving the lease. Maybe
> clarify this with
>
> "Lease grantor, only set if this struct drm_master represents a lessee
> holding a lease of objects from @lessor. Full owners of the device have
> this set to NULL."
>
>> + * drm_lease_create(). Each lessee holds a reference to its lessor that
>
> I also figured it'd be a good idea to link to the drm_lease docs here to
> explain the concepts, but alas we don't have those :-( Hence at least
> define what we mean with lessor, lessee, lease and owner.
>
Thanks for the suggestions, Daniel. I'll incorporate them in a v2.
Regarding the missing drm_lease docs... any reason we can't just add it
in? Seems like most of the comments in drm_lease.c are almost correctly
formatted too. I could clean them up, define the terms in a preamble,
then add it to the v2 patch.
>> + * it releases upon being destroyed in drm_lease_destroy().
>> + *
>> + * Display resource leases form a tree of &struct drm_master. All of
>
> For now we've limited the tree to a depth of 1, you can't create another
> lease if yourself are a lease. I guess another reason to update the
> drm_lease.c docs.
>
> So maybe add "Currently the lease tree depth is limited to 1."
>
>> + * these get activated simultaneously, so &drm_device.master
>> + * points at the top of the tree (for which lessor is NULL).
>> */
>> -
>> struct drm_master *lessor;
>> +
>> + /**
>> + * @lessee_id:
>> + *
>> + * ID for lessees. Owners always have ID 0. Protected by
>
> Maybe clarify to "Owners (i.e. @lessor is NULL) ..."
>
>> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
>> + */
>> int lessee_id;
>> +
>> + /**
>> + * @lessee_list:
>> + *
>> + * List of lessees of the same master. Protected by
>
> We try to distinguis between list entries and the list, even though it's
> the same struct. So maybe
>
> "List entry of lessees of @lessor, where they are linked to @lessee. Not
> use for owners."
>
>> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
>
>> + */
>> struct list_head lessee_list;
>> +
>> + /**
>> + * @lessees:
>> + *
>> + * List of drm_masters leasing from this one. Protected by
>> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
>> + *
>> + * This master cannot be destroyed unless this list is empty as lessors
>> + * are referenced by all their lessees.
>
> Maybe add "this list is empty of no leases have been granted."
>
>> + */
>> struct list_head lessees;
>> +
>> + /**
>> + * @leases:
>> + *
>> + * Objects leased to this drm_master. Protected by
>> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
>> + *
>> + * Objects are leased all together in drm_lease_create(), and are
>> + * removed all together when the lease is revoked.
>> + */
>> struct idr leases;
>> +
>> + /**
>> + * @lessee_idr:
>> + *
>> + * All lessees under this owner (only used where lessor is NULL).
>
> @lessor so it's highlighted correctly
>
>> + * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex.
>> + */
>> struct idr lessee_idr;
>> /* private: */
>
> With the nits addressed:
>
> Reviewed-by: Daniel Vetter <[email protected]>
>
> Thanks for updating the docs!
> -Daniel
>
>> #if IS_ENABLED(CONFIG_DRM_LEGACY)
>> --
>> 2.25.1
>>
>
On Thu, Jul 22, 2021 at 3:03 PM Desmond Cheong Zhi Xi
<[email protected]> wrote:
>
> On 22/7/21 6:35 pm, Daniel Vetter wrote:
> > On Thu, Jul 22, 2021 at 05:29:28PM +0800, Desmond Cheong Zhi Xi wrote:
> >> In particular, we make it clear that &drm_device.mode_config.idr_mutex
> >> protects the lease idr and list structures for drm_master. The lessor
> >> field itself doesn't need to be protected as it doesn't change after
> >> it's set in drm_lease_create.
> >>
> >> Additionally, we add descriptions for the lifetime of lessors and
> >> leases to make it easier to reason about them.
> >>
> >> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> >> ---
> >> include/drm/drm_auth.h | 62 ++++++++++++++++++++++++++++++++++--------
> >> 1 file changed, 51 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> >> index f99d3417f304..c978c85464fa 100644
> >> --- a/include/drm/drm_auth.h
> >> +++ b/include/drm/drm_auth.h
> >> @@ -58,12 +58,6 @@ struct drm_lock_data {
> >> * @refcount: Refcount for this master object.
> >> * @dev: Link back to the DRM device
> >> * @driver_priv: Pointer to driver-private information.
> >> - * @lessor: Lease holder
> >> - * @lessee_id: id for lessees. Owners always have id 0
> >> - * @lessee_list: other lessees of the same master
> >> - * @lessees: drm_masters leasing from this one
> >> - * @leases: Objects leased to this drm_master.
> >> - * @lessee_idr: All lessees under this owner (only used where lessor == NULL)
> >> *
> >> * Note that master structures are only relevant for the legacy/primary device
> >> * nodes, hence there can only be one per device, not one per drm_minor.
> >> @@ -88,17 +82,63 @@ struct drm_master {
> >> struct idr magic_map;
> >> void *driver_priv;
> >>
> >> - /* Tree of display resource leases, each of which is a drm_master struct
> >> - * All of these get activated simultaneously, so drm_device master points
> >> - * at the top of the tree (for which lessor is NULL). Protected by
> >> - * &drm_device.mode_config.idr_mutex.
> >> + /**
> >> + * @lessor:
> >> + *
> >> + * Lease holder. The lessor does not change once it's set in
> >
> > Lessor is the lease grantor, lessee is the one receiving the lease. Maybe
> > clarify this with
> >
> > "Lease grantor, only set if this struct drm_master represents a lessee
> > holding a lease of objects from @lessor. Full owners of the device have
> > this set to NULL."
> >
> >> + * drm_lease_create(). Each lessee holds a reference to its lessor that
> >
> > I also figured it'd be a good idea to link to the drm_lease docs here to
> > explain the concepts, but alas we don't have those :-( Hence at least
> > define what we mean with lessor, lessee, lease and owner.
> >
>
> Thanks for the suggestions, Daniel. I'll incorporate them in a v2.
>
> Regarding the missing drm_lease docs... any reason we can't just add it
> in? Seems like most of the comments in drm_lease.c are almost correctly
> formatted too. I could clean them up, define the terms in a preamble,
> then add it to the v2 patch.
Sure if you want to do that, that would be great. Usual style tips:
- We only document driver interfaces, so structs/inline functions in
headers and exported symbols in .c files.
- Anything else interesting just leave as normal C comments
- An overview DOC: section that explains a bit how leases work and why
(git blame on the commit that added it should help, otherwise I can
type that up) would also be really great.
I think the right place for this is in the drm-uapi.rst section after
the section about primary nodes:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#modeset-base-object-abstraction
Cheers, Daniel
>
> >> + * it releases upon being destroyed in drm_lease_destroy().
> >> + *
> >> + * Display resource leases form a tree of &struct drm_master. All of
> >
> > For now we've limited the tree to a depth of 1, you can't create another
> > lease if yourself are a lease. I guess another reason to update the
> > drm_lease.c docs.
> >
> > So maybe add "Currently the lease tree depth is limited to 1."
> >
> >> + * these get activated simultaneously, so &drm_device.master
> >> + * points at the top of the tree (for which lessor is NULL).
> >> */
> >> -
> >> struct drm_master *lessor;
> >> +
> >> + /**
> >> + * @lessee_id:
> >> + *
> >> + * ID for lessees. Owners always have ID 0. Protected by
> >
> > Maybe clarify to "Owners (i.e. @lessor is NULL) ..."
> >
> >> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> >> + */
> >> int lessee_id;
> >> +
> >> + /**
> >> + * @lessee_list:
> >> + *
> >> + * List of lessees of the same master. Protected by
> >
> > We try to distinguis between list entries and the list, even though it's
> > the same struct. So maybe
> >
> > "List entry of lessees of @lessor, where they are linked to @lessee. Not
> > use for owners."
> >
> >> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> >
> >> + */
> >> struct list_head lessee_list;
> >> +
> >> + /**
> >> + * @lessees:
> >> + *
> >> + * List of drm_masters leasing from this one. Protected by
> >> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> >> + *
> >> + * This master cannot be destroyed unless this list is empty as lessors
> >> + * are referenced by all their lessees.
> >
> > Maybe add "this list is empty of no leases have been granted."
> >
> >> + */
> >> struct list_head lessees;
> >> +
> >> + /**
> >> + * @leases:
> >> + *
> >> + * Objects leased to this drm_master. Protected by
> >> + * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> >> + *
> >> + * Objects are leased all together in drm_lease_create(), and are
> >> + * removed all together when the lease is revoked.
> >> + */
> >> struct idr leases;
> >> +
> >> + /**
> >> + * @lessee_idr:
> >> + *
> >> + * All lessees under this owner (only used where lessor is NULL).
> >
> > @lessor so it's highlighted correctly
> >
> >> + * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex.
> >> + */
> >> struct idr lessee_idr;
> >> /* private: */
> >
> > With the nits addressed:
> >
> > Reviewed-by: Daniel Vetter <[email protected]>
> >
> > Thanks for updating the docs!
> > -Daniel
> >
> >> #if IS_ENABLED(CONFIG_DRM_LEGACY)
> >> --
> >> 2.25.1
> >>
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 7/22/21 5:29 AM, Desmond Cheong Zhi Xi wrote:
> drm_file.master should be protected by either drm_device.master_mutex
> or drm_file.master_lookup_lock when being dereferenced. However,
> drm_master_get is called on unprotected file_priv->master pointers in
> vmw_surface_define_ioctl and vmw_gb_surface_define_internal.
>
> This is fixed by replacing drm_master_get with drm_file_get_master.
>
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
Reviewed-by: Zack Rusin <[email protected]>
Thanks for taking the time to fix this. Apart from the clear logic error, do you happen to know under what circumstances would this be hit? We have someone looking at writing some vmwgfx specific igt tests and I was wondering if I could add this to the list.
z
On 23/7/21 3:17 am, Zack Rusin wrote:
> On 7/22/21 5:29 AM, Desmond Cheong Zhi Xi wrote:
>> drm_file.master should be protected by either drm_device.master_mutex
>> or drm_file.master_lookup_lock when being dereferenced. However,
>> drm_master_get is called on unprotected file_priv->master pointers in
>> vmw_surface_define_ioctl and vmw_gb_surface_define_internal.
>>
>> This is fixed by replacing drm_master_get with drm_file_get_master.
>>
>> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
>
> Reviewed-by: Zack Rusin <[email protected]>
>
> Thanks for taking the time to fix this. Apart from the clear logic
> error, do you happen to know under what circumstances would this be hit?
> We have someone looking at writing some vmwgfx specific igt tests and I
> was wondering if I could add this to the list.
>
> z
Hi Zack,
Thanks for the review.
For some context, the use-after-free happens when there's a race between
accessing the value of drm_file.master, and a call to
drm_setmaster_ioctl. If drm_file is not the creator of master, then the
ioctl allocates a new master for drm_file and puts the old master.
Thus for example, the old value of drm_file.master could be freed in
between getting the value of file_priv->master, and the call to
drm_master_get.
I'm not entirely sure whether this scenario is a good candidate for a test?
For further reference, the issue was originally caught by Syzbot here:
https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803
And from the logs it seems that the reproducer set up a race between
DRM_IOCTL_GET_UNIQUE and DRM_IOCTL_SET_MASTER. So possibly a race
between VMW_CREATE_SURFACE and DRM_IOCTL_SET_MASTER could trigger the
same bug.
Best wishes,
Desmond