On Tue, Mar 3, 2026 at 4:29 PM Michel Dänzer <[email protected]> wrote:
>
> 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 <[email protected]>
> >>
> >> 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.
The intention of the original patch was to ignore the args->points
field if the flag is not set:
if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE)
point = args->point;
Using args->point unconditionally later was therefore a mistake.
>
>
> --
> Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer
> https://redhat.com \ Libre software enthusiast
