Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/3/26 15:59, Christian König wrote:
> On 3/3/26 15:53, Maarten Lankhorst wrote:
>> Hey,
>>
>> Den 2026-03-01 kl. 13:34, skrev Julian Orth:
>>> Consider the following application:
>>>
>>>     #include <fcntl.h>
>>>     #include <string.h>
>>>     #include <drm/drm.h>
>>>     #include <sys/ioctl.h>
>>>
>>>     int main(void) {
>>>         int fd = open("/dev/dri/renderD128", O_RDWR);
>>>         struct drm_syncobj_create arg1;
>>>         ioctl(fd, DRM_IOCTL_SYNCOBJ_CREATE, &arg1);
>>>         struct drm_syncobj_handle arg2;
>>>         memset(&arg2, 1, sizeof(arg2)); // simulate dirty stack
>>>         arg2.handle = arg1.handle;
>>>         arg2.flags = 0;
>>>         arg2.fd = 0;
>>>         arg2.pad = 0;
>>>         // arg2.point = 0; // userspace is required to set point to 0
>>>         ioctl(fd, DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, &arg2);
>>>     }
>>>
>>> The last ioctl returns EINVAL because args->point is not 0. However,
>>> userspace developed against older kernel versions is not aware of the
>>> new point field and might therefore not initialize it.
>>>
>>> The correct check would be
>>>
>>>     if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE)
>>>         return -EINVAL;
>>>
>>> However, there might already be userspace that relies on this not
>>> returning an error as long as point == 0. Therefore use the more lenient
>>> check.
>>>
>>> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
>>> Signed-off-by: Julian Orth <ju.orth@xxxxxxxxx>
>>
>> I'm not convinced this is the correct fix.
>> Userspace built before the change had the old size for drm_syncobj_create,
>> the size is encoded into the ioctl, and zero extended as needed.
>>
>> See drivers/gpu/drm/drm_ioctl.c:
>> 	out_size = in_size = _IOC_SIZE(cmd);
>> 	...
>> 	if (ksize > in_size)
>> 		memset(kdata + in_size, 0, ksize - in_size);
>>
>> This is a bug in a newly built app, and should be handled by explicitly zeroing
>> the entire struct or using named initializers, and only setting specific members
>> as required.
>>
>> In particular, apps built before the change will never encounter this bug.
> 
> Yeah, I've realized that after pushing the patch as well.
> 
> But I still think this patch is the right thing to do, because without requesting the functionality by setting the flag the point should clearly not have any effect at all.
> 
> And when an application would have only explicitly assigned the fields known previously and then later been compiled with the new points field it would have failed.
> 
> It is good practice to memset() structures given to the kernel so that all bytes are zero initialized, but it is not documented as mandatory as far as I know.

Even though it may not be documented, it is in fact mandatory. Otherwise it's not possible to safely extend ioctl structs in general.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast