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

[Julian Orth]

On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@xxxxxxxxxxx> wrote:
>
> On 3/3/26 18:44, Maarten Lankhorst wrote:
> > Den 2026-03-03 kl. 18:30, skrev Julian Orth:
> >> On Tue, Mar 3, 2026 at 6:18 PM Michel Dänzer <michel.daenzer@xxxxxxxxxxx> wrote:
> >>>
> >>> I wrote in my first post in this thread that I don't object to your patch, so you can relax and stop trying to convince me not to object to it. :)
> >>>
> >>> I'm just pointing out that this is working around broken user-space code, and that there are other similar cases where that kind of broken users-space code couldn't be worked around in the kernel, so it's better to also fix the user-space code anyway.
> >>
> >> At this point I think we're arguing about "how can ioctls be extended"
> >> and "does userspace have to use memset" in general, not just about
> >> this particular ioctl. You've made the argument that ioctls are not
> >> extensible in general unless userspace uses memset. However, I'm not
> >> yet convinced of this. As you've also said above, drm_ioctl happily
> >> truncates or zero-extends ioctl arguments without returning an error
> >> due to size mismatch. Therefore, the only way for userspace to detect
> >> if the kernel supports the "extended" ioctl is to add a flag so that
> >> the kernel can return an error if it doesn't know the flag. And then
> >> that flag could also be used by the kernel to detect which fields of
> >> the argument are potentially uninitialized.
> >>
> >> That is why I asked above if you knew of any other examples where an
> >> ioctl was extended and where memset(0) became effectively required due
> >> to the extension.
>
> Since it's always been effectively required for ioctl structs, "become" doesn't apply.
>
>
> In terms of documentation, the "(How to avoid) Botching up ioctls" page says under Basics:
>
> * Check all unused fields and flags and all the padding for whether it’s 0, and reject the ioctl if that’s not the case.
>
> Which is what the code you're modifying here did: The code after the args->point checks doesn't use the point field, so it's checking that user space initialized the field to 0 per above.

I don't believe that is true. The old code only checked args->point if
the flags argument was 0. If the flags argument contained
EXPORT_SYNC_FILE but not TIMELINE, then the old code ignored
args->point completely. The patch suggested by Maarten does what
you're suggesting: It unconditionally verifies that args->point does
not contain garbage. However, since the original code only used
args->points if TIMELINE was set, I believe the intention behind the
TIMELINE flag was to ignore the args->point field if the flag was
unset. That assumption is the basis for my patch.

>
> This contradicts the arguments in the commit log, so I'm leaning toward objecting to this patch now.
>
>
> > You don't even need to use memset, this would work too:
> >
> > struct drm_syncobj_handle args = {
> >       .flags = 0
> > };
>
> TL;DR: This method isn't 100% safe either.
>
> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.

I don't think this is workable. There is a lot of code out there that
effectively vendors the ioctl definitions (and so is not affected by
new fields) and relies on field-based initialization affecting all
significant bytes. For example
https://github.com/Smithay/drm-rs/blob/08dee22f0dcfa4a73a18ca3a954d4f7c2c749c03/drm-ffi/src/syncobj.rs#L48-L58.

>
> FWIW, libdrm uses memset for all ioctl structs.
>
>
> --
> Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
> https://redhat.com             \               Libre software enthusiast