[PATCH] drm/syncobj: Enforce strict checking of timeline syncobj struct

[PATCH] drm/syncobj: Enforce strict checking of timeline syncobj struct

[Maarten Lankhorst]

We add a new flag DRM_SYNCOBJ_*_FLAGS_TIMELINE and a point argument,
but we never check if the point is only set when the timeline flag
is set, and we still allow the timeline flag to be set when sync
files are not used.

This was discovered when userspace increased the size of the ioctl
to include args->point, but never cleared args->point, so fd_to_handle
and handle_to_fd ioctl's without timeline started failing.

Add more strict checking to prevent userspace from creating
new bugs!

Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs")
Cc: Rob Clark <robdclark@chromium.org>
Cc: Julian Orth <ju.orth@gmail.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Michel Dänzer <michel.daenzer@mailbox.org>
---
 drivers/gpu/drm/drm_syncobj.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 250734dee928e..33f2fc987e1d1 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -857,7 +857,6 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 	struct drm_syncobj_handle *args = data;
 	unsigned int valid_flags = DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE |
 				   DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE;
-	u64 point = 0;
 
 	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
 		return -EOPNOTSUPP;
@@ -868,14 +867,14 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 	if (args->flags & ~valid_flags)
 		return -EINVAL;
 
-	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE)
-		point = args->point;
+	if (!(args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) && args->point)
+		return -EINVAL;
 
 	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
 		return drm_syncobj_export_sync_file(file_private, args->handle,
-						    point, &args->fd);
+						    args->point, &args->fd);
 
-	if (args->point)
+	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE)
 		return -EINVAL;
 
 	return drm_syncobj_handle_to_fd(file_private, args->handle,
@@ -889,7 +888,6 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 	struct drm_syncobj_handle *args = data;
 	unsigned int valid_flags = DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE |
 				   DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE;
-	u64 point = 0;
 
 	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
 		return -EOPNOTSUPP;
@@ -900,16 +898,16 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 	if (args->flags & ~valid_flags)
 		return -EINVAL;
 
-	if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE)
-		point = args->point;
+	if (!(args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE) && args->point)
+		return -EINVAL;
 
 	if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
 		return drm_syncobj_import_sync_file_fence(file_private,
 							  args->fd,
 							  args->handle,
-							  point);
+							  args->point);
 
-	if (args->point)
+	if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_TIMELINE)
 		return -EINVAL;
 
 	return drm_syncobj_fd_to_handle(file_private, args->fd,
-- 
2.51.0

Claude review: drm/syncobj: Enforce strict checking of timeline syncobj struct

[Claude Code Review Bot]

Patch Review

**Approach:** The patch makes two logical changes in each of the two ioctl handlers (`handle_to_fd` and `fd_to_handle`):

1. **Reject `point` without `TIMELINE` flag** — Previously, if userspace passed a non-zero `args->point` without the `TIMELINE` flag, the `point` local variable was simply left at 0 and the non-zero `args->point` was silently ignored. Now it returns `-EINVAL`. This is the fix for the described userspace bug.

2. **Reject `TIMELINE` flag without sync file flag** — Previously, after the sync file export/import path, the code checked `if (args->point)` to reject timeline usage on the non-sync-file path. Now it checks `if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE)` instead. This is more correct since after change #1, `point` could legitimately be 0 with the `TIMELINE` flag set.

**Positive aspects:**
- The commit message clearly explains the motivation (userspace bug with uninitialized `args->point`).
- Removing the `point` local variable and using `args->point` directly simplifies the code.
- The `Fixes:` tag is appropriate.

**Concern — ABI break for existing userspace:**

The new check at line 870/901:
```c
if (!(args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) && args->point)
    return -EINVAL;
```

This will **break existing userspace** that has the exact bug described in the commit message — passing garbage in `args->point` without the `TIMELINE` flag. The old code silently ignored this; the new code fails with `-EINVAL`. While the intent is to force userspace to fix their bugs, this is technically a UAPI regression. Any deployed userspace with the uninitialized `args->point` bug that previously worked will now fail.

The kernel's usual stance is "don't break userspace." The commit message acknowledges this was "discovered when userspace increased the size of the ioctl to include args->point, but never cleared args->point," which suggests a real application is affected. The question is: should the kernel reject this (forcing a userspace fix) or continue to be permissive (as it was before)?

Given that the `TIMELINE` extensions are relatively new and the affected userspace is presumably being fixed concurrently, this is probably acceptable, but it would be worth confirming that the affected userspace fix is coordinated. A `Cc: stable` tag might also be appropriate if backporting is desired to prevent the buggy userspace pattern from becoming more entrenched.

**Minor nit — TIMELINE + EXPORT_SYNC_FILE with point=0:**

With this patch, `TIMELINE` flag + `EXPORT_SYNC_FILE` flag + `point=0` is now accepted and passed through to `drm_syncobj_export_sync_file()`. Previously this also worked (point would be set to 0 from `args->point`). This is fine — timeline point 0 typically means "current fence" — but worth noting that the semantics are preserved.

**Overall:** The patch is correct and well-motivated. The only substantive concern is the potential for breaking existing (buggy) userspace, which the commit message should address more explicitly — perhaps noting that the broken userspace in question is being fixed in tandem, or adding a comment about the deliberate ABI tightening.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/syncobj: Enforce strict checking of timeline syncobj struct

[Claude Code Review Bot]

Overall Series Review

Subject: drm/syncobj: Enforce strict checking of timeline syncobj struct
Author: Maarten Lankhorst <dev@lankhorst.se>
Patches: 1
Reviewed: 2026-03-12T06:50:35.480324

---

This is a single patch that tightens input validation for the DRM syncobj `handle_to_fd` and `fd_to_handle` ioctls. The motivation is sound: userspace was able to set `args->point` without the `TIMELINE` flag, or set the `TIMELINE` flag without the `EXPORT_SYNC_FILE`/`IMPORT_SYNC_FILE` flag, leading to silent misbehavior. The fix is clean and well-structured.

However, there is one potential **userspace ABI regression concern** that deserves discussion.

---
Generated by Claude Code Patch Reviewer