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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

Den 2026-03-03 kl. 20:12, skrev Julian Orth:
> On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@mailbox.org> 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@mailbox.org> 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.

Vendoring is not affected. When you carry your own definition, it's your
responsibility for updating the users if you update the struct too.

Using rust here is a good example. The rust code would still use the old definition.
If you use it against the new definition it fails to compile, or use default valuesif asked.
You would notice your newly introduced bug at compile time.

Kind regards,
~Maarten Lankhorst