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

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

[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@gmail.com>
---
This patch fixes a regression that would cause conversions between
syncobj handles and fds to fail if userspace did not initialize a
recently-added field to 0.
---
 drivers/gpu/drm/drm_syncobj.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 250734dee928..49eccb43ce63 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -875,7 +875,7 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 		return drm_syncobj_export_sync_file(file_private, args->handle,
 						    point, &args->fd);
 
-	if (args->point)
+	if (point)
 		return -EINVAL;
 
 	return drm_syncobj_handle_to_fd(file_private, args->handle,
@@ -909,7 +909,7 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 							  args->handle,
 							  point);
 
-	if (args->point)
+	if (point)
 		return -EINVAL;
 
 	return drm_syncobj_fd_to_handle(file_private, args->fd,

---
base-commit: eb71ab2bf72260054677e348498ba995a057c463
change-id: 20260301-point-4305b6417f55

Best regards,
-- 
Julian Orth <ju.orth@gmail.com>

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

[Christian König]

On 3/1/26 13:34, Julian Orth wrote:
> 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@gmail.com>

Good catch, Reviewed-by: Christian König <christian.koenig@amd.com>

As long as nobody objects I'm going to push this to drm-misc-fixes later today.

Thanks,
Christian.

> ---
> This patch fixes a regression that would cause conversions between
> syncobj handles and fds to fail if userspace did not initialize a
> recently-added field to 0.
> ---
>  drivers/gpu/drm/drm_syncobj.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 250734dee928..49eccb43ce63 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -875,7 +875,7 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>                 return drm_syncobj_export_sync_file(file_private, args->handle,
>                                                     point, &args->fd);
> 
> -       if (args->point)
> +       if (point)
>                 return -EINVAL;
> 
>         return drm_syncobj_handle_to_fd(file_private, args->handle,
> @@ -909,7 +909,7 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
>                                                           args->handle,
>                                                           point);
> 
> -       if (args->point)
> +       if (point)
>                 return -EINVAL;
> 
>         return drm_syncobj_fd_to_handle(file_private, args->fd,
> 
> ---
> base-commit: eb71ab2bf72260054677e348498ba995a057c463
> change-id: 20260301-point-4305b6417f55
> 
> Best regards,
> --
> Julian Orth <ju.orth@gmail.com>
> 

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

[Dmitry Osipenko]

On 3/2/26 14:27, Christian König wrote:
> On 3/1/26 13:34, Julian Orth wrote:
>> 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@gmail.com>
> 
> Good catch, Reviewed-by: Christian König <christian.koenig@amd.com>
> 
> As long as nobody objects I'm going to push this to drm-misc-fixes later today.

No objections, thanks

-- 
Best regards,
Dmitry

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

[Claude Code Review Bot]

Patch Review

**Analysis of the fix**:

The code in both handlers follows the same pattern:

```c
u64 point = 0;
// ...
if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE)
    point = args->point;
// ... export_sync_file early return ...
if (args->point)   // <-- BUG: checks raw field even when TIMELINE not set
    return -EINVAL;
return drm_syncobj_handle_to_fd(file_private, args->handle, &args->fd);
```

The local variable `point` already captures the correct semantic intent: it's `0` unless the `TIMELINE` flag is set, in which case it copies `args->point`. The change from `if (args->point)` to `if (point)` means:

- **TIMELINE not set**: `point` is always `0`, check passes regardless of stack garbage in `args->point`. This is correct — when TIMELINE is not requested, the `point` field is meaningless and should be ignored.
- **TIMELINE set**: `point == args->point`, so the check is equivalent to the old behavior.

No security concern arises because the `point` value is not used in the subsequent `drm_syncobj_handle_to_fd()` / `drm_syncobj_fd_to_handle()` calls.

**Comments**:

1. **Correct and minimal fix**. The change is the smallest possible fix that addresses the regression. Using the already-existing local `point` variable is elegant — no new logic is needed.

2. **Commit message is well-written**. The example program clearly demonstrates the regression, the explanation of why the "correct" fix (`if (args->flags & TIMELINE) return -EINVAL`) can't be used is sound — it could break userspace that passes TIMELINE + point==0 and expects success.

3. **Missing `Cc: stable`**. Given this is a regression fix with a `Fixes:` tag, it should include `Cc: stable@vger.kernel.org` to ensure it gets backported to stable kernels that carry commit c2d3a7300695.

4. **Both ioctls fixed symmetrically**. The fix correctly applies to both `drm_syncobj_handle_to_fd_ioctl` and `drm_syncobj_fd_to_handle_ioctl`, which have the same pattern and the same bug. Good.

5. **Minor observation on the commit message**: The comment says "userspace is required to set point to 0" but with this fix, userspace is no longer required to set `point` to 0 when not using TIMELINE — that's the whole point of the fix. The commit message is clear about this being the intent, so this is just an observation, not a problem.

Overall: **Reviewed-by worthy**. Clean, correct, minimal regression fix.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Overall Series Review

Subject: Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack
Author: =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
Patches: 3
Reviewed: 2026-03-03T13:58:23.267122

---

This is a single-patch fix for a real regression introduced by commit c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs"). The analysis in the commit message is correct, the fix is minimal and appropriate, and it addresses both affected ioctl handlers symmetrically.

**The regression**: When the `point` field was added to `struct drm_syncobj_handle`, both `drm_syncobj_handle_to_fd_ioctl` and `drm_syncobj_fd_to_handle_ioctl` began checking `if (args->point) return -EINVAL;` on the non-timeline code paths. This breaks userspace that is compiled against new headers but doesn't initialize the `point` field (because it was written for the older interface where `point` didn't exist). The `point` field picks up stack garbage, fails the check, and the ioctl returns `-EINVAL`.

Note: Userspace compiled against the *old* headers (without `point`) is unaffected because the DRM ioctl infrastructure zero-pads the struct when `in_size < drv_size` (see `drm_ioctl()` at `drm_ioctl.c:898-899`). The regression specifically affects userspace compiled against new headers but not updated to initialize `point`.

**Recommendation**: This patch looks correct and ready to apply. It should also be tagged with `Cc: stable@vger.kernel.org` for backporting.

---
Generated by Claude Code Patch Reviewer