On 11/25/25 14:02, Sokolowski, Jan wrote: >> -----Original Message----- >> From: Christian König <[email protected]> >> Sent: Tuesday, November 25, 2025 1:54 PM >> To: Sokolowski, Jan <[email protected]>; dri- >> [email protected] >> Cc: David Francis <[email protected]>; Maarten Lankhorst >> <[email protected]>; Maxime Ripard >> <[email protected]>; Thomas Zimmermann <[email protected]>; >> David Airlie <[email protected]>; Simona Vetter <[email protected]>; Felix >> Kuehling <[email protected]>; De Marchi, Lucas >> <[email protected]> >> Subject: Re: [PATCH 1/1] drm: disallow setting 0 as new handle in >> DRM_IOCTL_GEM_CHANGE_HANDLE >> >> On 11/25/25 11:28, Jan Sokolowski wrote: >>> drm_file's object_idr uses 1 as base value, which can cause id >>> mismatch when trying to use DRM_IOCTL_GEM_CHANGE_HANDLE >>> to change id from 1 to 0. >>> >>> Disallow 0 as new handle in that ioctl. >>> >>> Fixes: 53096728b891 ("drm: Add DRM prime interface to reassign GEM >> handle") >>> Signed-off-by: Jan Sokolowski <[email protected]> >>> Cc: David Francis <[email protected]> >>> Cc: Maarten Lankhorst <[email protected]> >>> Cc: Maxime Ripard <[email protected]> >>> Cc: Thomas Zimmermann <[email protected]> >>> Cc: David Airlie <[email protected]> >>> Cc: Simona Vetter <[email protected]> >>> Cc: "Christian König" <[email protected]> >>> Cc: Felix Kuehling <[email protected]> >>> Cc: Lucas De Marchi <[email protected]> >>> --- >>> drivers/gpu/drm/drm_gem.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >>> index 68168d58a7c8..2a49a8e396fa 100644 >>> --- a/drivers/gpu/drm/drm_gem.c >>> +++ b/drivers/gpu/drm/drm_gem.c >>> @@ -975,6 +975,10 @@ int drm_gem_change_handle_ioctl(struct >> drm_device *dev, void *data, >>> if (args->handle == args->new_handle) >>> return 0; >>> >>> + /* As the idr base is 1, trying to set handle 0 will create id mismatch >> */ >>> + if (args->new_handle == 0) >>> + return 0; >> >> That would need to return -EINVAl or some other error code. > > Ok, will change that in next version. > >> >> But I'm wondering why that is necessary at all? Doesn't idr_alloc() return an >> error when you try to allocate handle 0? > > It appears that idr_alloc doesn't return any errors in this scenario, > otherwise we'd goto out_unlock straight away.
Mhm, that is a bit misleading. We intentionally initialized the idr so that it starts at 1. Maybe idr_alloc() should be fixed instead. But that is clearly a much larger change, with the return code fixed feel free to add Reviewed-by: Christian König <[email protected]> to this patch here. Adding a CC: stable tag sound appropriate as well. Regards, Christian. > >> >> Regards, >> Christian. >> >>> + >>> mutex_lock(&file_priv->prime.lock); >>> >>> spin_lock(&file_priv->table_lock); >
