From: Carsten Haitzler carsten.haitzler@arm.com
This series starts to enable purecap support for drm ioctls. This series enables all the libdrm tests (tested and working). You will also need the purecap libdrm port as well to complement this.
Carsten Haitzler (3): drm: Fix copy to/from user so that caps transport in the region drm: Fix purecap vblank handling drm: fix up purecap handling of all iotcls in libdrm test tools
drivers/gpu/drm/drm_atomic_uapi.c | 98 +++++++++++++++++------ drivers/gpu/drm/drm_connector.c | 108 +++++++++++++++++++------ drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_ioctl.c | 6 +- drivers/gpu/drm/drm_mode_config.c | 126 +++++++++++++++++++++--------- drivers/gpu/drm/drm_mode_object.c | 37 ++++++++- drivers/gpu/drm/drm_plane.c | 89 +++++++++++++++++---- drivers/gpu/drm/drm_property.c | 89 +++++++++++++++++---- drivers/gpu/drm/drm_vblank.c | 39 ++++++--- include/drm/drm_vblank.h | 23 ++++++ include/uapi/drm/drm.h | 6 +- include/uapi/drm/drm_mode.h | 42 +++++----- tools/include/uapi/drm/drm.h | 2 +- 13 files changed, 506 insertions(+), 161 deletions(-)
From: Carsten Haitzler carsten.haitzler@arm.com
DRM makes copies of ioctl payload data to/from userspace and some of this can also have pointers to other memory (or capabilities for morello) and thus it needs to assume the worst and copy while retaining capabilities. This also requires making sure the stack buffer for small ioctls (128 bytes or less) is aligned to the maximum pointer or capability size.
Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com --- drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_ioctl.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 446458aca8e9..76e3616158f3 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -651,7 +651,7 @@ ssize_t drm_read(struct file *filp, char __user *buffer, break; }
- if (copy_to_user(buffer + ret, e->event, length)) { + if (copy_to_user_with_ptr(buffer + ret, e->event, length)) { if (ret == 0) ret = -EFAULT; goto put_back_event; diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index f2cda382f8ed..b303d6a88285 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -821,7 +821,7 @@ long drm_ioctl(struct file *filp, drm_ioctl_t *func; unsigned int nr = DRM_IOCTL_NR(cmd); int retcode = -EINVAL; - char stack_kdata[128]; + char stack_kdata[128] __attribute__((aligned(__alignof__(__kernel_uintptr_t)))); char *kdata = NULL; unsigned int in_size, out_size, drv_size, ksize; bool is_driver_ioctl; @@ -884,7 +884,7 @@ long drm_ioctl(struct file *filp, } }
- if (copy_from_user(kdata, (void __user *)arg, in_size) != 0) { + if (copy_from_user_with_ptr(kdata, (void __user *)arg, in_size) != 0) { retcode = -EFAULT; goto err_i1; } @@ -893,7 +893,7 @@ long drm_ioctl(struct file *filp, memset(kdata + in_size, 0, ksize - in_size);
retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags); - if (copy_to_user((void __user *)arg, kdata, out_size) != 0) + if (copy_to_user_with_ptr((void __user *)arg, kdata, out_size) != 0) retcode = -EFAULT;
err_i1:
From: Carsten Haitzler carsten.haitzler@arm.com
Vblank is a bit different because it produces events that then can copy pointers/capabilities back to userspace via a read on the DRM device as opposed to the ioctl. The compat handler called the normal native/purecap one and this happened to work for compat but not for purecap. Make the shared "native" purecap handling work for both and share that infra properly to avoid copy and paste.
Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com --- drivers/gpu/drm/drm_atomic_uapi.c | 98 ++++++++++++++++++++++++------- drivers/gpu/drm/drm_vblank.c | 39 ++++++++---- include/drm/drm_vblank.h | 23 ++++++++ include/uapi/drm/drm.h | 6 +- include/uapi/drm/drm_mode.h | 12 ++-- 5 files changed, 136 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 3e4668a20157..11c5515039d4 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -940,7 +940,9 @@ int drm_atomic_get_property(struct drm_mode_object *obj, */
static struct drm_pending_vblank_event *create_vblank_event( - struct drm_crtc *crtc, uint64_t user_data) + struct drm_crtc *crtc, + __kernel_uintptr_t user_data, + bool compat) { struct drm_pending_vblank_event *e = NULL;
@@ -950,8 +952,14 @@ static struct drm_pending_vblank_event *create_vblank_event(
e->event.base.type = DRM_EVENT_FLIP_COMPLETE; e->event.base.length = sizeof(e->event); - e->event.vbl.crtc_id = crtc->base.id; - e->event.vbl.user_data = user_data; + e->compat = compat; + if (compat) { + e->event.vbl32.crtc_id = crtc->base.id; + e->event.vbl32.user_data = user_data; + } else { + e->event.vbl.crtc_id = crtc->base.id; + e->event.vbl.user_data = user_data; + }
return e; } @@ -1148,20 +1156,43 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state, return 0; }
+struct drm_mode_atomic32 { + __u32 flags; + __u32 count_objs; + __u64 objs_ptr; + __u64 count_props_ptr; + __u64 props_ptr; + __u64 prop_values_ptr; + __u64 reserved; + __u64 user_data; +}; + static int prepare_signaling(struct drm_device *dev, struct drm_atomic_state *state, - struct drm_mode_atomic *arg, + void *arg_data, struct drm_file *file_priv, struct drm_out_fence_state **fence_state, unsigned int *num_fences) { + struct drm_mode_atomic32 *arg32 = arg_data; + struct drm_mode_atomic *arg = arg_data; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_connector *conn; struct drm_connector_state *conn_state; int i, c = 0, ret; + __u32 flags; + __kernel_uintptr_t user_data; + + if (in_compat64_syscall()) { + flags = arg32->flags; + user_data = (__kernel_uintptr_t)compat_ptr(arg32->user_data); + } else { + flags = arg->flags; + user_data = arg->user_data; + }
- if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) + if (flags & DRM_MODE_ATOMIC_TEST_ONLY) return 0;
for_each_new_crtc_in_state(state, crtc, crtc_state, i) { @@ -1169,17 +1200,18 @@ static int prepare_signaling(struct drm_device *dev,
fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) { + if (flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) { struct drm_pending_vblank_event *e;
- e = create_vblank_event(crtc, arg->user_data); + e = create_vblank_event(crtc, user_data, + in_compat64_syscall()); if (!e) return -ENOMEM;
crtc_state->event = e; }
- if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { + if (flags & DRM_MODE_PAGE_FLIP_EVENT) { struct drm_pending_vblank_event *e = crtc_state->event;
if (!file_priv) @@ -1265,7 +1297,7 @@ static int prepare_signaling(struct drm_device *dev, * Having this flag means user mode pends on event which will never * reach due to lack of at least one CRTC for signaling */ - if (c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) { + if (c == 0 && (flags & DRM_MODE_PAGE_FLIP_EVENT)) { drm_dbg_atomic(dev, "need at least one CRTC for DRM_MODE_PAGE_FLIP_EVENT"); return -EINVAL; } @@ -1326,17 +1358,21 @@ static void complete_signaling(struct drm_device *dev, int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_atomic32 *arg32 = data; struct drm_mode_atomic *arg = data; - uint32_t __user *objs_ptr = uaddr_to_user_ptr(arg->objs_ptr); - uint32_t __user *count_props_ptr = uaddr_to_user_ptr(arg->count_props_ptr); - uint32_t __user *props_ptr = uaddr_to_user_ptr(arg->props_ptr); - uint64_t __user *prop_values_ptr = uaddr_to_user_ptr(arg->prop_values_ptr); unsigned int copied_objs, copied_props; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; struct drm_out_fence_state *fence_state; int ret = 0; unsigned int i, j, num_fences; + __u32 flags; + __u32 count_objs; + uint32_t __user *objs_ptr; + uint32_t __user *count_props_ptr; + uint32_t __user *props_ptr; + uint64_t __user *prop_values_ptr; + void __user *reserved;
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1352,25 +1388,43 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, return -EINVAL; }
- if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) { + if (in_compat64_syscall()) { + flags = arg32->flags; + count_objs = arg32->count_objs; + objs_ptr = compat_ptr(arg32->objs_ptr); + count_props_ptr = compat_ptr(arg32->count_props_ptr); + props_ptr = compat_ptr(arg32->props_ptr); + prop_values_ptr = compat_ptr(arg32->prop_values_ptr); + reserved = compat_ptr(arg32->reserved); + } else { + flags = arg->flags; + count_objs = arg->count_objs; + objs_ptr = (uint32_t __user *)arg->objs_ptr; + count_props_ptr = (uint32_t __user *)arg->count_props_ptr; + props_ptr = (uint32_t __user *)arg->props_ptr; + prop_values_ptr = (uint64_t __user *)arg->prop_values_ptr; + reserved = (void __user *)arg->reserved; + } + + if (flags & ~DRM_MODE_ATOMIC_FLAGS) { drm_dbg_atomic(dev, "commit failed: invalid flag\n"); return -EINVAL; }
- if (arg->reserved) { + if (reserved) { drm_dbg_atomic(dev, "commit failed: reserved field set\n"); return -EINVAL; }
- if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) { + if (flags & DRM_MODE_PAGE_FLIP_ASYNC) { drm_dbg_atomic(dev, "commit failed: invalid flag DRM_MODE_PAGE_FLIP_ASYNC\n"); return -EINVAL; }
/* can't test and expect an event at the same time. */ - if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) && - (arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) { + if ((flags & DRM_MODE_ATOMIC_TEST_ONLY) && + (flags & DRM_MODE_PAGE_FLIP_EVENT)) { drm_dbg_atomic(dev, "commit failed: page-flip event requested with test-only commit\n"); return -EINVAL; @@ -1382,7 +1436,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); state->acquire_ctx = &ctx; - state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET); + state->allow_modeset = !!(flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
retry: copied_objs = 0; @@ -1390,7 +1444,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, fence_state = NULL; num_fences = 0;
- for (i = 0; i < arg->count_objs; i++) { + for (i = 0; i < count_objs; i++) { uint32_t obj_id, count_props; struct drm_mode_object *obj;
@@ -1468,9 +1522,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, if (ret) goto out;
- if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) { + if (flags & DRM_MODE_ATOMIC_TEST_ONLY) { ret = drm_atomic_check_only(state); - } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) { + } else if (flags & DRM_MODE_ATOMIC_NONBLOCK) { ret = drm_atomic_nonblocking_commit(state); } else { ret = drm_atomic_commit(state); diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 877e2067534f..fbd60acc5eda 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1031,14 +1031,20 @@ static void send_vblank_event(struct drm_device *dev, case DRM_EVENT_VBLANK: case DRM_EVENT_FLIP_COMPLETE: tv = ktime_to_timespec64(now); - e->event.vbl.sequence = seq; /* * e->event is a user space structure, with hardcoded unsigned * 32-bit seconds/microseconds. This is safe as we always use * monotonic timestamps since linux-4.15 */ - e->event.vbl.tv_sec = tv.tv_sec; - e->event.vbl.tv_usec = tv.tv_nsec / 1000; + if (e->compat) { + e->event.vbl32.sequence = seq; + e->event.vbl32.tv_sec = tv.tv_sec; + e->event.vbl32.tv_usec = tv.tv_nsec / 1000; + } else { + e->event.vbl.sequence = seq; + e->event.vbl.tv_sec = tv.tv_sec; + e->event.vbl.tv_usec = tv.tv_nsec / 1000; + } break; case DRM_EVENT_CRTC_SEQUENCE: if (seq) @@ -1659,10 +1665,13 @@ int drm_legacy_modeset_ctl_ioctl(struct drm_device *dev, void *data, static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe, u64 req_seq, union drm_wait_vblank *vblwait, - struct drm_file *file_priv) + struct drm_file *file_priv, + bool compat + ) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; struct drm_pending_vblank_event *e; + struct drm_crtc *crtc = NULL; ktime_t now; u64 seq; int ret; @@ -1675,12 +1684,19 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
e->pipe = pipe; e->event.base.type = DRM_EVENT_VBLANK; - e->event.base.length = sizeof(e->event.vbl); - e->event.vbl.user_data = vblwait->request.signal; - e->event.vbl.crtc_id = 0; - if (drm_core_check_feature(dev, DRIVER_MODESET)) { - struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); - + if (drm_core_check_feature(dev, DRIVER_MODESET)) + crtc = drm_crtc_from_index(dev, pipe); + e->compat = compat; + if (compat) { + e->event.base.length = sizeof(e->event.vbl32); + e->event.vbl32.user_data = vblwait->request.signal; + e->event.vbl32.crtc_id = 0; + if (crtc) + e->event.vbl32.crtc_id = crtc->base.id; + } else { + e->event.base.length = sizeof(e->event.vbl); + e->event.vbl.user_data = vblwait->request.signal; + e->event.vbl.crtc_id = 0; if (crtc) e->event.vbl.crtc_id = crtc->base.id; } @@ -1886,7 +1902,8 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, /* must hold on to the vblank ref until the event fires * drm_vblank_put will be called asynchronously */ - return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, file_priv); + return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, + file_priv, in_compat64_syscall()); }
if (req_seq != seq) { diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index 7f3957943dd1..ccbcea75ba13 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -36,6 +36,22 @@ struct drm_device; struct drm_crtc; struct drm_vblank_work;
+struct drm_event_vblank32 { + struct drm_event base; + __u64 user_data; + __u32 tv_sec; + __u32 tv_usec; + __u32 sequence; + __u32 crtc_id; /* 0 on older kernels that do not support this */ +}; + +struct drm_event_crtc_sequence32 { + struct drm_event base; + __u64 user_data; + __s64 time_ns; + __u64 sequence; +}; + /** * struct drm_pending_vblank_event - pending vblank event tracking */ @@ -52,6 +68,10 @@ struct drm_pending_vblank_event { * @sequence: frame event should be triggered at */ u64 sequence; + /** + * @compat: This is a compat event + */ + bool compat; /** * @event: Actual event which will be sent to userspace. */ @@ -75,6 +95,9 @@ struct drm_pending_vblank_event { * @event.seq: Event payload for the MODE_QUEUEU_SEQUENCE IOCTL. */ struct drm_event_crtc_sequence seq; + + struct drm_event_vblank32 vbl32; + struct drm_event_crtc_sequence32 seq32; } event; };
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index de723566c5ae..c5cc98abbffe 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -494,7 +494,7 @@ enum drm_vblank_seq_type { struct drm_wait_vblank_request { enum drm_vblank_seq_type type; unsigned int sequence; - unsigned long signal; + __kernel_uintptr_t signal; };
struct drm_wait_vblank_reply { @@ -971,7 +971,7 @@ struct drm_crtc_queue_sequence { __u32 crtc_id; __u32 flags; __u64 sequence; /* on input, target sequence. on output, actual sequence */ - __u64 user_data; /* user data passed to event */ + __kernel_uintptr_t user_data; /* user data passed to event */ };
#if defined(__cplusplus) @@ -1277,7 +1277,7 @@ struct drm_event {
struct drm_event_vblank { struct drm_event base; - __u64 user_data; + __kernel_uintptr_t user_data; __u32 tv_sec; __u32 tv_usec; __u32 sequence; diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 128d09138ceb..7f9aaa0bf03d 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -1130,12 +1130,12 @@ struct drm_mode_destroy_dumb { struct drm_mode_atomic { __u32 flags; __u32 count_objs; - __u64 objs_ptr; - __u64 count_props_ptr; - __u64 props_ptr; - __u64 prop_values_ptr; - __u64 reserved; - __u64 user_data; + __kernel_uintptr_t objs_ptr; + __kernel_uintptr_t count_props_ptr; + __kernel_uintptr_t props_ptr; + __kernel_uintptr_t prop_values_ptr; + __kernel_uintptr_t reserved; + __kernel_uintptr_t user_data; };
struct drm_format_modifier_blob {
On 15/04/2024 16:48, carsten.haitzler@foss.arm.com wrote:
[...]
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {
- struct drm_mode_atomic32 *arg32 = data; struct drm_mode_atomic *arg = data;
- uint32_t __user *objs_ptr = uaddr_to_user_ptr(arg->objs_ptr);
- uint32_t __user *count_props_ptr = uaddr_to_user_ptr(arg->count_props_ptr);
- uint32_t __user *props_ptr = uaddr_to_user_ptr(arg->props_ptr);
- uint64_t __user *prop_values_ptr = uaddr_to_user_ptr(arg->prop_values_ptr); unsigned int copied_objs, copied_props; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; struct drm_out_fence_state *fence_state; int ret = 0; unsigned int i, j, num_fences;
- __u32 flags;
- __u32 count_objs;
- uint32_t __user *objs_ptr;
- uint32_t __user *count_props_ptr;
- uint32_t __user *props_ptr;
- uint64_t __user *prop_values_ptr;
- void __user *reserved;
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1352,25 +1388,43 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, return -EINVAL; }
- if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
- if (in_compat64_syscall()) {
flags = arg32->flags;count_objs = arg32->count_objs;objs_ptr = compat_ptr(arg32->objs_ptr);count_props_ptr = compat_ptr(arg32->count_props_ptr);props_ptr = compat_ptr(arg32->props_ptr);prop_values_ptr = compat_ptr(arg32->prop_values_ptr);reserved = compat_ptr(arg32->reserved);- } else {
flags = arg->flags;count_objs = arg->count_objs;objs_ptr = (uint32_t __user *)arg->objs_ptr;count_props_ptr = (uint32_t __user *)arg->count_props_ptr;props_ptr = (uint32_t __user *)arg->props_ptr;prop_values_ptr = (uint64_t __user *)arg->prop_values_ptr;reserved = (void __user *)arg->reserved;
Not sure it makes sense to treat reserved as a pointer. Can't we keep it as a __u64?
Happy with how things are looking otherwise.
Kevin
On 4/15/24 4:13 PM, Kevin Brodsky wrote:
On 15/04/2024 16:48, carsten.haitzler@foss.arm.com wrote:
[...]
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {
- struct drm_mode_atomic32 *arg32 = data; struct drm_mode_atomic *arg = data;
- uint32_t __user *objs_ptr = uaddr_to_user_ptr(arg->objs_ptr);
- uint32_t __user *count_props_ptr = uaddr_to_user_ptr(arg->count_props_ptr);
- uint32_t __user *props_ptr = uaddr_to_user_ptr(arg->props_ptr);
- uint64_t __user *prop_values_ptr = uaddr_to_user_ptr(arg->prop_values_ptr); unsigned int copied_objs, copied_props; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; struct drm_out_fence_state *fence_state; int ret = 0; unsigned int i, j, num_fences;
- __u32 flags;
- __u32 count_objs;
- uint32_t __user *objs_ptr;
- uint32_t __user *count_props_ptr;
- uint32_t __user *props_ptr;
- uint64_t __user *prop_values_ptr;
- void __user *reserved;
/* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1352,25 +1388,43 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, return -EINVAL; }
- if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
- if (in_compat64_syscall()) {
flags = arg32->flags;count_objs = arg32->count_objs;objs_ptr = compat_ptr(arg32->objs_ptr);count_props_ptr = compat_ptr(arg32->count_props_ptr);props_ptr = compat_ptr(arg32->props_ptr);prop_values_ptr = compat_ptr(arg32->prop_values_ptr);reserved = compat_ptr(arg32->reserved);- } else {
flags = arg->flags;count_objs = arg->count_objs;objs_ptr = (uint32_t __user *)arg->objs_ptr;count_props_ptr = (uint32_t __user *)arg->count_props_ptr;props_ptr = (uint32_t __user *)arg->props_ptr;prop_values_ptr = (uint64_t __user *)arg->prop_values_ptr;reserved = (void __user *)arg->reserved;Not sure it makes sense to treat reserved as a pointer. Can't we keep it as a __u64?
Happy with how things are looking otherwise.
alignment will force it to have to use up a pointer/cap of space anyway... it is as if this was considered for future use as a pointer so making it a __u64 won't change memory layout or struct size, so may as well go with the worst case - a ptr. :) the code very much wants this to NOT be set to anything. if we make it a __u64 we won't be testing the content of the padding space being zero... :)
On 15/04/2024 18:18, Carsten Haitzler wrote:
On 4/15/24 4:13 PM, Kevin Brodsky wrote:
On 15/04/2024 16:48, carsten.haitzler@foss.arm.com wrote:
[...]
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_atomic32 *arg32 = data; struct drm_mode_atomic *arg = data; - uint32_t __user *objs_ptr = uaddr_to_user_ptr(arg->objs_ptr); - uint32_t __user *count_props_ptr = uaddr_to_user_ptr(arg->count_props_ptr); - uint32_t __user *props_ptr = uaddr_to_user_ptr(arg->props_ptr); - uint64_t __user *prop_values_ptr = uaddr_to_user_ptr(arg->prop_values_ptr); unsigned int copied_objs, copied_props; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; struct drm_out_fence_state *fence_state; int ret = 0; unsigned int i, j, num_fences; + __u32 flags; + __u32 count_objs; + uint32_t __user *objs_ptr; + uint32_t __user *count_props_ptr; + uint32_t __user *props_ptr; + uint64_t __user *prop_values_ptr; + void __user *reserved; /* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1352,25 +1388,43 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, return -EINVAL; } - if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) { + if (in_compat64_syscall()) { + flags = arg32->flags; + count_objs = arg32->count_objs; + objs_ptr = compat_ptr(arg32->objs_ptr); + count_props_ptr = compat_ptr(arg32->count_props_ptr); + props_ptr = compat_ptr(arg32->props_ptr); + prop_values_ptr = compat_ptr(arg32->prop_values_ptr); + reserved = compat_ptr(arg32->reserved); + } else { + flags = arg->flags; + count_objs = arg->count_objs; + objs_ptr = (uint32_t __user *)arg->objs_ptr; + count_props_ptr = (uint32_t __user *)arg->count_props_ptr; + props_ptr = (uint32_t __user *)arg->props_ptr; + prop_values_ptr = (uint64_t __user *)arg->prop_values_ptr; + reserved = (void __user *)arg->reserved;
Not sure it makes sense to treat reserved as a pointer. Can't we keep it as a __u64?
Happy with how things are looking otherwise.
alignment will force it to have to use up a pointer/cap of space anyway... it is as if this was considered for future use as a pointer so making it a __u64 won't change memory layout or struct size, so may as well go with the worst case - a ptr. :) the code very much wants this to NOT be set to anything. if we make it a __u64 we won't be testing the content of the padding space being zero... :)
I suspected this was for padding reasons. We never take this into account when expanding structs, that's a decision we made a while back. Otherwise, there would be explicit padding all over the BPF structs we changed, for instance. That's just not worth the hassle for our scope. FWIW you are already introducing implicit padding in most of the DRM structs you are changing, including in this very struct (64 bits between count_objs and objs_ptr).
It's also worth noting that you are not actually checking that all 128 bits are zeroes in PCuABI. if (reserved) will only check that the address is 0, i.e. the bottom 64 bits.
Let's keep it simple with reserved as __u64.
Kevin
On 4/16/24 8:08 AM, Kevin Brodsky wrote:
On 15/04/2024 18:18, Carsten Haitzler wrote:
On 4/15/24 4:13 PM, Kevin Brodsky wrote:
On 15/04/2024 16:48, carsten.haitzler@foss.arm.com wrote:
[...]
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_atomic32 *arg32 = data; struct drm_mode_atomic *arg = data; - uint32_t __user *objs_ptr = uaddr_to_user_ptr(arg->objs_ptr); - uint32_t __user *count_props_ptr = uaddr_to_user_ptr(arg->count_props_ptr); - uint32_t __user *props_ptr = uaddr_to_user_ptr(arg->props_ptr); - uint64_t __user *prop_values_ptr = uaddr_to_user_ptr(arg->prop_values_ptr); unsigned int copied_objs, copied_props; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; struct drm_out_fence_state *fence_state; int ret = 0; unsigned int i, j, num_fences; + __u32 flags; + __u32 count_objs; + uint32_t __user *objs_ptr; + uint32_t __user *count_props_ptr; + uint32_t __user *props_ptr; + uint64_t __user *prop_values_ptr; + void __user *reserved; /* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1352,25 +1388,43 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, return -EINVAL; } - if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) { + if (in_compat64_syscall()) { + flags = arg32->flags; + count_objs = arg32->count_objs; + objs_ptr = compat_ptr(arg32->objs_ptr); + count_props_ptr = compat_ptr(arg32->count_props_ptr); + props_ptr = compat_ptr(arg32->props_ptr); + prop_values_ptr = compat_ptr(arg32->prop_values_ptr); + reserved = compat_ptr(arg32->reserved); + } else { + flags = arg->flags; + count_objs = arg->count_objs; + objs_ptr = (uint32_t __user *)arg->objs_ptr; + count_props_ptr = (uint32_t __user *)arg->count_props_ptr; + props_ptr = (uint32_t __user *)arg->props_ptr; + prop_values_ptr = (uint64_t __user *)arg->prop_values_ptr; + reserved = (void __user *)arg->reserved;
Not sure it makes sense to treat reserved as a pointer. Can't we keep it as a __u64?
Happy with how things are looking otherwise.
alignment will force it to have to use up a pointer/cap of space anyway... it is as if this was considered for future use as a pointer so making it a __u64 won't change memory layout or struct size, so may as well go with the worst case - a ptr. :) the code very much wants this to NOT be set to anything. if we make it a __u64 we won't be testing the content of the padding space being zero... :)
I suspected this was for padding reasons. We never take this into account when expanding structs, that's a decision we made a while back. Otherwise, there would be explicit padding all over the BPF structs we changed, for instance. That's just not worth the hassle for our scope.
I'm not so sure this is for padding. It's named reserved not pad. Additionally the code inside the ioctl EXPLICITLY checks the reserved field is empty so it's been written expecting this to be used in future but making sure no one puts "junk" in there now for when it does so as to not break ABI if/when that day comes (the kernel can assume previous users have it all 0, and new users of this field set it to something non-zero if using it). If it was just for alignment/padding, it'd not do this.
If I made it anything other than a ptr, the code that checks for this being empty would only check a portion of the "padding" space for emptiness and not all of it. I'm not changing this drm logic here. It's still there and I'm keeping with the spirit of the the previous pre-purecap code. That is to check the reserved space between prop_values_ptr and user_data is set to 0. Imagine a hypothetical 18bit address arch which allows unaligned access to anything. If this were a __u64 then we'd not have the space for a pointer here as a compiler could place user_data at an unaligned address immediately after this __u64 reserved field.
FWIW you are already introducing implicit padding in most of the DRM structs you are changing, including in this very struct (64 bits between count_objs and objs_ptr).
It's also worth noting that you are not actually checking that all 128 bits are zeroes in PCuABI. if (reserved) will only check that the address is 0, i.e. the bottom 64 bits.
That is a good point. I probably need to fix that. In a portable way preferably with no ifdef...
Let's keep it simple with reserved as __u64.
I think that's wrong (see above). Let's assume this was a non-capability 128bit architecture. __u64's are mostly used to store pointers (in drm ioctls). If I see a __u64 that isn't clearly used for something else than a ptr, I need to assume it is (or could be). Certainly I need to assume there must be a field with enough space for a pointer as this screams out to me that that was the intent.
On 16/04/2024 10:08, Carsten Haitzler wrote:
On 4/16/24 8:08 AM, Kevin Brodsky wrote:
On 15/04/2024 18:18, Carsten Haitzler wrote:
On 4/15/24 4:13 PM, Kevin Brodsky wrote:
On 15/04/2024 16:48, carsten.haitzler@foss.arm.com wrote:
[...]
int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_atomic32 *arg32 = data; struct drm_mode_atomic *arg = data; - uint32_t __user *objs_ptr = uaddr_to_user_ptr(arg->objs_ptr); - uint32_t __user *count_props_ptr = uaddr_to_user_ptr(arg->count_props_ptr); - uint32_t __user *props_ptr = uaddr_to_user_ptr(arg->props_ptr); - uint64_t __user *prop_values_ptr = uaddr_to_user_ptr(arg->prop_values_ptr); unsigned int copied_objs, copied_props; struct drm_atomic_state *state; struct drm_modeset_acquire_ctx ctx; struct drm_out_fence_state *fence_state; int ret = 0; unsigned int i, j, num_fences; + __u32 flags; + __u32 count_objs; + uint32_t __user *objs_ptr; + uint32_t __user *count_props_ptr; + uint32_t __user *props_ptr; + uint64_t __user *prop_values_ptr; + void __user *reserved; /* disallow for drivers not supporting atomic: */ if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) @@ -1352,25 +1388,43 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, return -EINVAL; } - if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) { + if (in_compat64_syscall()) { + flags = arg32->flags; + count_objs = arg32->count_objs; + objs_ptr = compat_ptr(arg32->objs_ptr); + count_props_ptr = compat_ptr(arg32->count_props_ptr); + props_ptr = compat_ptr(arg32->props_ptr); + prop_values_ptr = compat_ptr(arg32->prop_values_ptr); + reserved = compat_ptr(arg32->reserved); + } else { + flags = arg->flags; + count_objs = arg->count_objs; + objs_ptr = (uint32_t __user *)arg->objs_ptr; + count_props_ptr = (uint32_t __user *)arg->count_props_ptr; + props_ptr = (uint32_t __user *)arg->props_ptr; + prop_values_ptr = (uint64_t __user *)arg->prop_values_ptr; + reserved = (void __user *)arg->reserved;
Not sure it makes sense to treat reserved as a pointer. Can't we keep it as a __u64?
Happy with how things are looking otherwise.
alignment will force it to have to use up a pointer/cap of space anyway... it is as if this was considered for future use as a pointer so making it a __u64 won't change memory layout or struct size, so may as well go with the worst case - a ptr. :) the code very much wants this to NOT be set to anything. if we make it a __u64 we won't be testing the content of the padding space being zero... :)
I suspected this was for padding reasons. We never take this into account when expanding structs, that's a decision we made a while back. Otherwise, there would be explicit padding all over the BPF structs we changed, for instance. That's just not worth the hassle for our scope.
I'm not so sure this is for padding. It's named reserved not pad. Additionally the code inside the ioctl EXPLICITLY checks the reserved field is empty so it's been written expecting this to be used in future but making sure no one puts "junk" in there now for when it does so as to not break ABI if/when that day comes (the kernel can assume previous users have it all 0, and new users of this field set it to something non-zero if using it). If it was just for alignment/padding, it'd not do this.
If I made it anything other than a ptr, the code that checks for this being empty would only check a portion of the "padding" space for emptiness and not all of it. I'm not changing this drm logic here. It's still there and I'm keeping with the spirit of the the previous pre-purecap code. That is to check the reserved space between prop_values_ptr and user_data is set to 0. Imagine a hypothetical 18bit address arch which allows unaligned access to anything. If this were a __u64 then we'd not have the space for a pointer here as a compiler could place user_data at an unaligned address immediately after this __u64 reserved field.
FWIW you are already introducing implicit padding in most of the DRM structs you are changing, including in this very struct (64 bits between count_objs and objs_ptr).
It's also worth noting that you are not actually checking that all 128 bits are zeroes in PCuABI. if (reserved) will only check that the address is 0, i.e. the bottom 64 bits.
That is a good point. I probably need to fix that. In a portable way preferably with no ifdef...
Let's keep it simple with reserved as __u64.
I think that's wrong (see above). Let's assume this was a non-capability 128bit architecture. __u64's are mostly used to store pointers (in drm ioctls). If I see a __u64 that isn't clearly used for something else than a ptr, I need to assume it is (or could be). Certainly I need to assume there must be a field with enough space for a pointer as this screams out to me that that was the intent.
Discussed offline - agreed to keep it as a __kernel_uintptr_t, as it could potentially be used as a pointer, but compat_ptr() shouldn't be used as we don't actually want to create a pointer. user_ptr_is_same() [1] can be used to check that a whole pointer is zeroes.
Kevin
[1] https://git.morello-project.org/morello/kernel/linux/-/blob/morello/master/D...
From: Carsten Haitzler carsten.haitzler@arm.com
The modetest, proptest vbltest tools from drm all work with this as the ioctls they use with this patch now handle purecap ABI. The general design is for these ioctls to have an import + export code "blob" that converts to to/from in-kernel-data based on if it's a compat or purecap caller and embedded compat structs together with the ioctl handling. This is less invasive and less copy + paste based than alternative solutions, thus making it more maintainable as a patchset. There is no need to have special different ioctl number handling as drm already handles this in generic infrastructure to make sure the compat and native purecap ioctl numbers map to the same ioctl func unless its explicitly in the 32bit (compat) handling.
Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com --- drivers/gpu/drm/drm_connector.c | 108 +++++++++++++++++++------ drivers/gpu/drm/drm_mode_config.c | 126 +++++++++++++++++++++--------- drivers/gpu/drm/drm_mode_object.c | 37 ++++++++- drivers/gpu/drm/drm_plane.c | 89 +++++++++++++++++---- drivers/gpu/drm/drm_property.c | 89 +++++++++++++++++---- include/uapi/drm/drm_mode.h | 30 +++---- tools/include/uapi/drm/drm.h | 2 +- 7 files changed, 366 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index bc052b147172..e5838057b833 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -2883,6 +2883,26 @@ drm_mode_expose_to_userspace(const struct drm_display_mode *mode, int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_connector32 { + __u64 encoders_ptr; + __u64 modes_ptr; + __u64 props_ptr; + __u64 prop_values_ptr; + __u32 count_modes; + __u32 count_props; + __u32 count_encoders; + + __u32 encoder_id; + __u32 connector_id; + __u32 connector_type; + __u32 connector_type_id; + __u32 connection; + __u32 mm_width; + __u32 mm_height; + __u32 subpixel; + __u32 pad; + }; + struct drm_mode_get_connector32 *out_resp32 = data; struct drm_mode_get_connector *out_resp = data; struct drm_connector *connector; struct drm_encoder *encoder; @@ -2895,21 +2915,46 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr; bool is_current_master; + __u32 connector_id; + __u32 count_encoders; + __u32 count_modes; + __u32 count_props; + uint32_t __user *prop_ptr; + uint64_t __user *prop_values; + + if (in_compat64_syscall()) { + connector_id = out_resp32->connector_id; + count_encoders = out_resp32->count_encoders; + count_modes = out_resp32->count_modes; + count_props = out_resp32->count_props; + encoder_ptr = compat_ptr(out_resp32->encoders_ptr); + mode_ptr = compat_ptr(out_resp32->modes_ptr); + prop_ptr = compat_ptr(out_resp32->props_ptr); + prop_values = compat_ptr(out_resp32->prop_values_ptr); + } else { + connector_id = out_resp->connector_id; + count_encoders = out_resp->count_encoders; + count_modes = out_resp->count_modes; + count_props = out_resp->count_props; + encoder_ptr = (uint32_t __user *)(out_resp->encoders_ptr); + mode_ptr = (struct drm_mode_modeinfo __user *)(out_resp->modes_ptr); + prop_ptr = (uint32_t __user *)(out_resp->props_ptr); + prop_values = (uint64_t __user *)(out_resp->prop_values_ptr); + }
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
memset(&u_mode, 0, sizeof(struct drm_mode_modeinfo));
- connector = drm_connector_lookup(dev, file_priv, out_resp->connector_id); + connector = drm_connector_lookup(dev, file_priv, connector_id); if (!connector) return -ENOENT;
encoders_count = hweight32(connector->possible_encoders);
- if ((out_resp->count_encoders >= encoders_count) && encoders_count) { + if ((count_encoders >= encoders_count) && encoders_count) { copied = 0; - encoder_ptr = uaddr_to_user_ptr(out_resp->encoders_ptr);
drm_connector_for_each_possible_encoder(connector, encoder) { if (put_user(encoder->base.id, encoder_ptr + copied)) { @@ -2919,16 +2964,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, copied++; } } - out_resp->count_encoders = encoders_count; - - out_resp->connector_id = connector->base.id; - out_resp->connector_type = connector->connector_type; - out_resp->connector_type_id = connector->connector_type_id; - is_current_master = drm_is_current_master(file_priv);
mutex_lock(&dev->mode_config.mutex); - if (out_resp->count_modes == 0) { + if (count_modes == 0) { if (is_current_master) connector->funcs->fill_modes(connector, dev->mode_config.max_width, @@ -2938,11 +2977,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, connector->base.id, connector->name); }
- out_resp->mm_width = connector->display_info.width_mm; - out_resp->mm_height = connector->display_info.height_mm; - out_resp->subpixel = connector->display_info.subpixel_order; - out_resp->connection = connector->status; - /* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head) { WARN_ON(mode->expose_to_userspace); @@ -2958,9 +2992,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */ - if ((out_resp->count_modes >= mode_count) && mode_count) { + if ((count_modes >= mode_count) && mode_count) { copied = 0; - mode_ptr = uaddr_to_user_ptr(out_resp->modes_ptr); list_for_each_entry(mode, &connector->modes, head) { if (!mode->expose_to_userspace) continue; @@ -2998,25 +3031,50 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, mode->expose_to_userspace = false; }
- out_resp->count_modes = mode_count; mutex_unlock(&dev->mode_config.mutex);
drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); encoder = drm_connector_get_encoder(connector); - if (encoder) - out_resp->encoder_id = encoder->base.id; - else - out_resp->encoder_id = 0;
/* Only grab properties after probing, to make sure EDID and other * properties reflect the latest status. */ ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, - uaddr_to_user_ptr(out_resp->props_ptr), - uaddr_to_user_ptr(out_resp->prop_values_ptr), - &out_resp->count_props); + prop_ptr, prop_values, + &count_props); drm_modeset_unlock(&dev->mode_config.connection_mutex);
+ if (in_compat64_syscall()) { + out_resp32->count_encoders = encoders_count; + out_resp32->count_modes = mode_count; + out_resp32->count_props = count_props; + out_resp32->connector_id = connector->base.id; + out_resp32->connector_type = connector->connector_type; + out_resp32->connector_type_id = connector->connector_type_id; + if (encoder) + out_resp32->encoder_id = encoder->base.id; + else + out_resp32->encoder_id = 0; + out_resp32->mm_width = connector->display_info.width_mm; + out_resp32->mm_height = connector->display_info.height_mm; + out_resp32->subpixel = connector->display_info.subpixel_order; + out_resp32->connection = connector->status; + } else { + out_resp->count_encoders = encoders_count; + out_resp->count_modes = mode_count; + out_resp->count_props = count_props; + out_resp->connector_id = connector->base.id; + out_resp->connector_type = connector->connector_type; + out_resp->connector_type_id = connector->connector_type_id; + if (encoder) + out_resp->encoder_id = encoder->base.id; + else + out_resp->encoder_id = 0; + out_resp->mm_width = connector->display_info.width_mm; + out_resp->mm_height = connector->display_info.height_mm; + out_resp->subpixel = connector->display_info.subpixel_order; + out_resp->connection = connector->status; + } out: drm_connector_put(connector);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 8525ef851540..80d195328b77 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -92,11 +92,30 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_card_res *card_res = data; + struct drm_mode_card_res32 { + __u64 fb_id_ptr; + __u64 crtc_id_ptr; + __u64 connector_id_ptr; + __u64 encoder_id_ptr; + __u32 count_fbs; + __u32 count_crtcs; + __u32 count_connectors; + __u32 count_encoders; + __u32 min_width; + __u32 max_width; + __u32 min_height; + __u32 max_height; + }; + struct drm_mode_card_res32 *card_res32 = data; struct drm_framebuffer *fb; struct drm_connector *connector; struct drm_crtc *crtc; struct drm_encoder *encoder; - int count, ret = 0; + int count_fb = 0, count_fbs; + int count_crtc = 0, count_crtcs; + int count_encoder = 0, count_encoders; + int count_connector = 0, count_connectors; + int ret = 0; uint32_t __user *fb_id; uint32_t __user *crtc_id; uint32_t __user *connector_id; @@ -107,49 +126,54 @@ int drm_mode_getresources(struct drm_device *dev, void *data, return -EOPNOTSUPP;
mutex_lock(&file_priv->fbs_lock); - count = 0; - fb_id = u64_to_user_ptr(card_res->fb_id_ptr); + if (in_compat64_syscall()) { + fb_id = compat_ptr(card_res32->fb_id_ptr); + crtc_id = compat_ptr(card_res32->crtc_id_ptr); + encoder_id = compat_ptr(card_res32->encoder_id_ptr); + connector_id = compat_ptr(card_res32->connector_id_ptr); + + count_fbs = card_res32->count_fbs; + count_crtcs = card_res32->count_crtcs; + count_encoders = card_res32->count_encoders; + count_connectors = card_res32->count_connectors; + } else { + fb_id = (uint32_t __user *)(card_res->fb_id_ptr); + crtc_id = (uint32_t __user *)(card_res->crtc_id_ptr); + encoder_id = (uint32_t __user *)(card_res->encoder_id_ptr); + connector_id = (uint32_t __user *)(card_res->connector_id_ptr); + + count_fbs = card_res->count_fbs; + count_crtcs = card_res->count_crtcs; + count_encoders = card_res->count_encoders; + count_connectors = card_res->count_connectors; + } list_for_each_entry(fb, &file_priv->fbs, filp_head) { - if (count < card_res->count_fbs && - put_user(fb->base.id, fb_id + count)) { - mutex_unlock(&file_priv->fbs_lock); - return -EFAULT; + if (count_fb < count_fbs && + put_user(fb->base.id, fb_id + count_fb)) { + ret = -EFAULT; + goto error; } - count++; + count_fb++; } - card_res->count_fbs = count; - mutex_unlock(&file_priv->fbs_lock); - - card_res->max_height = dev->mode_config.max_height; - card_res->min_height = dev->mode_config.min_height; - card_res->max_width = dev->mode_config.max_width; - card_res->min_width = dev->mode_config.min_width; - - count = 0; - crtc_id = u64_to_user_ptr(card_res->crtc_id_ptr); drm_for_each_crtc(crtc, dev) { if (drm_lease_held(file_priv, crtc->base.id)) { - if (count < card_res->count_crtcs && - put_user(crtc->base.id, crtc_id + count)) - return -EFAULT; - count++; + if (count_crtc < count_crtcs && + put_user(crtc->base.id, crtc_id + count_crtc)) { + ret = -EFAULT; + goto error; + } + count_crtc++; } } - card_res->count_crtcs = count; - - count = 0; - encoder_id = u64_to_user_ptr(card_res->encoder_id_ptr); drm_for_each_encoder(encoder, dev) { - if (count < card_res->count_encoders && - put_user(encoder->base.id, encoder_id + count)) - return -EFAULT; - count++; + if (count_encoder < count_encoders && + put_user(encoder->base.id, encoder_id + count_encoder)) { + ret = -EFAULT; + goto error; + } + count_encoder++; } - card_res->count_encoders = count; - drm_connector_list_iter_begin(dev, &conn_iter); - count = 0; - connector_id = u64_to_user_ptr(card_res->connector_id_ptr); drm_for_each_connector_iter(connector, &conn_iter) { /* only expose writeback connectors if userspace understands them */ if (!file_priv->writeback_connectors && @@ -157,17 +181,41 @@ int drm_mode_getresources(struct drm_device *dev, void *data, continue;
if (drm_lease_held(file_priv, connector->base.id)) { - if (count < card_res->count_connectors && - put_user(connector->base.id, connector_id + count)) { + if (count_connector < count_connectors && + put_user(connector->base.id, connector_id + count_connector)) { drm_connector_list_iter_end(&conn_iter); - return -EFAULT; + ret = -EFAULT; + goto error; } - count++; + count_connector++; } } - card_res->count_connectors = count; drm_connector_list_iter_end(&conn_iter);
+ if (in_compat64_syscall()) { + card_res32->count_fbs = count_fb; + card_res32->count_crtcs = count_crtc; + card_res32->count_encoders = count_encoder; + card_res32->count_connectors = count_connector; + + card_res32->max_height = dev->mode_config.max_height; + card_res32->min_height = dev->mode_config.min_height; + card_res32->max_width = dev->mode_config.max_width; + card_res32->min_width = dev->mode_config.min_width; + } else { + card_res->count_fbs = count_fb; + card_res->count_crtcs = count_crtc; + card_res->count_encoders = count_encoder; + card_res->count_connectors = count_connector; + + card_res->max_height = dev->mode_config.max_height; + card_res->min_height = dev->mode_config.min_height; + card_res->max_width = dev->mode_config.max_width; + card_res->min_width = dev->mode_config.min_width; + } + +error: + mutex_unlock(&file_priv->fbs_lock); return ret; }
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index 29df6cbd9c99..10b1c74214e0 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -435,17 +435,41 @@ int drm_mode_object_get_properties(struct drm_mode_object *obj, bool atomic, int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_obj_get_properties32 { + __u64 props_ptr; + __u64 prop_values_ptr; + __u32 count_props; + __u32 obj_id; + __u32 obj_type; + }; + struct drm_mode_obj_get_properties32 *arg32 = data; struct drm_mode_obj_get_properties *arg = data; struct drm_mode_object *obj; struct drm_modeset_acquire_ctx ctx; int ret = 0; + __u32 obj_id, obj_type, count_props; + uint32_t __user *props_ptr; + uint64_t __user *prop_values_ptr;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); + if (in_compat64_syscall()) { + count_props = arg32->count_props; + obj_id = arg32->obj_id; + obj_type = arg32->obj_type; + props_ptr = compat_ptr(arg32->props_ptr); + prop_values_ptr = compat_ptr(arg32->prop_values_ptr); + } else { + count_props = arg->count_props; + obj_id = arg->obj_id; + obj_type = arg->obj_type; + props_ptr = (uint32_t __user *)(arg->props_ptr); + prop_values_ptr = (uint64_t __user *)(arg->prop_values_ptr); + }
- obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type); + obj = drm_mode_object_find(dev, file_priv, obj_id, obj_type); if (!obj) { ret = -ENOENT; goto out; @@ -456,13 +480,18 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, }
ret = drm_mode_object_get_properties(obj, file_priv->atomic, - uaddr_to_user_ptr(arg->props_ptr), - uaddr_to_user_ptr(arg->prop_values_ptr), - &arg->count_props); + props_ptr, + prop_values_ptr, + &count_props);
out_unref: drm_mode_object_put(obj); out: + if (in_compat64_syscall()) { + arg32->count_props = count_props; + } else { + arg->count_props = count_props; + } DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); return ret; } diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 3dba57a39197..71407a42cf34 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -655,15 +655,27 @@ EXPORT_SYMBOL(drm_mode_plane_set_obj_prop); int drm_mode_getplane_res(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_plane_res32 { + __u64 plane_id_ptr; + __u32 count_planes; + }; + struct drm_mode_get_plane_res32 *plane_resp32 = data; struct drm_mode_get_plane_res *plane_resp = data; struct drm_plane *plane; uint32_t __user *plane_ptr; int count = 0; + __u32 count_planes;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- plane_ptr = u64_to_user_ptr(plane_resp->plane_id_ptr); + if (in_compat64_syscall()) { + plane_ptr = compat_ptr(plane_resp32->plane_id_ptr); + count_planes = plane_resp32->count_planes; + } else { + plane_ptr = (uint32_t __user *)(plane_resp->plane_id_ptr); + count_planes = plane_resp->count_planes; + }
/* * This ioctl is called twice, once to determine how much space is @@ -679,13 +691,18 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, continue;
if (drm_lease_held(file_priv, plane->base.id)) { - if (count < plane_resp->count_planes && + if (count < count_planes && put_user(plane->base.id, plane_ptr + count)) return -EFAULT; count++; } } - plane_resp->count_planes = count; + + if (in_compat64_syscall()) { + plane_resp32->count_planes = count; + } else { + plane_resp->count_planes = count; + }
return 0; } @@ -693,53 +710,91 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, int drm_mode_getplane(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_plane32 { + __u32 plane_id; + + __u32 crtc_id; + __u32 fb_id; + + __u32 possible_crtcs; + __u32 gamma_size; + + __u32 count_format_types; + __u64 format_type_ptr; + }; + struct drm_mode_get_plane32 *plane_resp32 = data; struct drm_mode_get_plane *plane_resp = data; struct drm_plane *plane; uint32_t __user *format_ptr; + __u32 plane_id, count_format_types, crtc_id, fb_id, possible_crtcs, gamma_size;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- plane = drm_plane_find(dev, file_priv, plane_resp->plane_id); + if (in_compat64_syscall()) { + format_ptr = compat_ptr(plane_resp32->format_type_ptr); + plane_id = plane_resp32->plane_id; + count_format_types = plane_resp32->count_format_types; + } else { + format_ptr = (uint32_t __user *)plane_resp->format_type_ptr; + plane_id = plane_resp->plane_id; + count_format_types = plane_resp->count_format_types; + } + + plane = drm_plane_find(dev, file_priv, plane_id); if (!plane) return -ENOENT;
drm_modeset_lock(&plane->mutex, NULL); if (plane->state && plane->state->crtc && drm_lease_held(file_priv, plane->state->crtc->base.id)) - plane_resp->crtc_id = plane->state->crtc->base.id; + crtc_id = plane->state->crtc->base.id; else if (!plane->state && plane->crtc && drm_lease_held(file_priv, plane->crtc->base.id)) - plane_resp->crtc_id = plane->crtc->base.id; + crtc_id = plane->crtc->base.id; else - plane_resp->crtc_id = 0; + crtc_id = 0;
if (plane->state && plane->state->fb) - plane_resp->fb_id = plane->state->fb->base.id; + fb_id = plane->state->fb->base.id; else if (!plane->state && plane->fb) - plane_resp->fb_id = plane->fb->base.id; + fb_id = plane->fb->base.id; else - plane_resp->fb_id = 0; + fb_id = 0; drm_modeset_unlock(&plane->mutex);
- plane_resp->plane_id = plane->base.id; - plane_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv, - plane->possible_crtcs); + plane_id = plane->base.id; + possible_crtcs = drm_lease_filter_crtcs(file_priv, + plane->possible_crtcs);
- plane_resp->gamma_size = 0; + gamma_size = 0;
/* * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */ if (plane->format_count && - (plane_resp->count_format_types >= plane->format_count)) { - format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr); + (count_format_types >= plane->format_count)) { if (copy_to_user(format_ptr, plane->format_types, sizeof(uint32_t) * plane->format_count)) { return -EFAULT; } } - plane_resp->count_format_types = plane->format_count; + + if (in_compat64_syscall()) { + plane_resp32->crtc_id = crtc_id; + plane_resp32->fb_id = fb_id; + plane_resp32->plane_id = plane_id; + plane_resp32->possible_crtcs = possible_crtcs; + plane_resp32->gamma_size = gamma_size; + plane_resp32->count_format_types = count_format_types; + } else { + plane_resp->crtc_id = crtc_id; + plane_resp->fb_id = fb_id; + plane_resp->plane_id = plane_id; + plane_resp->possible_crtcs = possible_crtcs; + plane_resp->gamma_size = gamma_size; + plane_resp->count_format_types = count_format_types; + }
return 0; } diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index dfec479830e4..18435fc9df97 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -457,6 +457,16 @@ EXPORT_SYMBOL(drm_property_destroy); int drm_mode_getproperty_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_property32 { + __u64 values_ptr; + __u64 enum_blob_ptr; + __u32 prop_id; + __u32 flags; + char name[DRM_PROP_NAME_LEN]; + __u32 count_values; + __u32 count_enum_blobs; + }; + struct drm_mode_get_property32 *out_resp32 = data; struct drm_mode_get_property *out_resp = data; struct drm_property *property; int enum_count = 0; @@ -465,36 +475,51 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, struct drm_property_enum *prop_enum; struct drm_mode_property_enum __user *enum_ptr; uint64_t __user *values_ptr; + __u32 prop_id, flags, count_values, count_enum_blobs; + char *name;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- property = drm_property_find(dev, file_priv, out_resp->prop_id); + if (in_compat64_syscall()) { + values_ptr = compat_ptr(out_resp32->values_ptr); + enum_ptr = compat_ptr(out_resp32->enum_blob_ptr); + prop_id = out_resp32->prop_id; + name = out_resp32->name; + count_values = out_resp32->count_values; + count_enum_blobs = out_resp32->count_enum_blobs; + } else { + values_ptr = (uint64_t __user *)out_resp->values_ptr; + enum_ptr = (struct drm_mode_property_enum __user *)out_resp->enum_blob_ptr; + prop_id = out_resp->prop_id; + name = out_resp->name; + count_values = out_resp->count_values; + count_enum_blobs = out_resp->count_enum_blobs; + } + + property = drm_property_find(dev, file_priv, prop_id); if (!property) return -ENOENT;
- strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN); - out_resp->flags = property->flags; + strscpy_pad(name, property->name, DRM_PROP_NAME_LEN); + flags = property->flags;
value_count = property->num_values; - values_ptr = u64_to_user_ptr(out_resp->values_ptr);
for (i = 0; i < value_count; i++) { - if (i < out_resp->count_values && + if (i < count_values && put_user(property->values[i], values_ptr + i)) { return -EFAULT; } } - out_resp->count_values = value_count;
copied = 0; - enum_ptr = u64_to_user_ptr(out_resp->enum_blob_ptr);
if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) || drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) { list_for_each_entry(prop_enum, &property->enum_list, head) { enum_count++; - if (out_resp->count_enum_blobs < enum_count) + if (count_enum_blobs < enum_count) continue;
if (copy_to_user(&enum_ptr[copied].value, @@ -506,7 +531,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, return -EFAULT; copied++; } - out_resp->count_enum_blobs = enum_count; + count_enum_blobs = enum_count; }
/* @@ -518,7 +543,19 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, * the property itself. */ if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) - out_resp->count_enum_blobs = 0; + count_enum_blobs = 0; + + if (in_compat64_syscall()) { + out_resp32->prop_id = prop_id; + out_resp32->flags = flags; + out_resp32->count_values = count_values; + out_resp32->count_enum_blobs = count_enum_blobs; + } else { + out_resp->prop_id = prop_id; + out_resp->flags = flags; + out_resp->count_values = count_values; + out_resp->count_enum_blobs = count_enum_blobs; + }
return 0; } @@ -754,29 +791,53 @@ EXPORT_SYMBOL(drm_property_replace_blob); int drm_mode_getblob_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_mode_get_blob32 { + __u32 blob_id; + __u32 length; + __u64 data; + }; + struct drm_mode_get_blob32 *out_resp32 = data; struct drm_mode_get_blob *out_resp = data; struct drm_property_blob *blob; int ret = 0; + __u32 blob_id; + __u32 length; + uint8_t __user *blob_data; + + if (in_compat64_syscall()) { + blob_id = out_resp32->blob_id; + length = out_resp32->length; + blob_data = compat_ptr(out_resp32->data); + } else { + blob_id = out_resp->blob_id; + length = out_resp->length; + blob_data = (uint8_t __user *)(out_resp->data); + }
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;
- blob = drm_property_lookup_blob(dev, out_resp->blob_id); + blob = drm_property_lookup_blob(dev, blob_id); if (!blob) return -ENOENT;
- if (out_resp->length == blob->length) { - if (copy_to_user(u64_to_user_ptr(out_resp->data), + if (length == blob->length) { + if (copy_to_user(blob_data, blob->data, blob->length)) { ret = -EFAULT; goto unref; } } - out_resp->length = blob->length; + length = blob->length; unref: drm_property_blob_put(blob);
+ if (in_compat64_syscall()) { + out_resp32->length = length; + } else { + out_resp->length = length; + } return ret; }
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 7f9aaa0bf03d..72fe5d89eede 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -260,10 +260,10 @@ struct drm_mode_modeinfo { };
struct drm_mode_card_res { - __u64 fb_id_ptr; - __u64 crtc_id_ptr; - __u64 connector_id_ptr; - __u64 encoder_id_ptr; + __kernel_uintptr_t fb_id_ptr; + __kernel_uintptr_t crtc_id_ptr; + __kernel_uintptr_t connector_id_ptr; + __kernel_uintptr_t encoder_id_ptr; __u32 count_fbs; __u32 count_crtcs; __u32 count_connectors; @@ -354,11 +354,11 @@ struct drm_mode_get_plane { * @format_type_ptr: Pointer to ``__u32`` array of formats that are * supported by the plane. These formats do not require modifiers. */ - __u64 format_type_ptr; + __kernel_uintptr_t format_type_ptr; };
struct drm_mode_get_plane_res { - __u64 plane_id_ptr; + __kernel_uintptr_t plane_id_ptr; __u32 count_planes; };
@@ -457,13 +457,13 @@ enum drm_mode_subconnector { */ struct drm_mode_get_connector { /** @encoders_ptr: Pointer to ``__u32`` array of object IDs. */ - __u64 encoders_ptr; + __kernel_uintptr_t encoders_ptr; /** @modes_ptr: Pointer to struct drm_mode_modeinfo array. */ - __u64 modes_ptr; + __kernel_uintptr_t modes_ptr; /** @props_ptr: Pointer to ``__u32`` array of property IDs. */ - __u64 props_ptr; + __kernel_uintptr_t props_ptr; /** @prop_values_ptr: Pointer to ``__u64`` array of property values. */ - __u64 prop_values_ptr; + __kernel_uintptr_t prop_values_ptr;
/** @count_modes: Number of modes. */ __u32 count_modes; @@ -589,9 +589,9 @@ struct drm_mode_property_enum { */ struct drm_mode_get_property { /** @values_ptr: Pointer to a ``__u64`` array. */ - __u64 values_ptr; + __kernel_uintptr_t values_ptr; /** @enum_blob_ptr: Pointer to a struct drm_mode_property_enum array. */ - __u64 enum_blob_ptr; + __kernel_uintptr_t enum_blob_ptr;
/** * @prop_id: Object ID of the property which should be retrieved. Set @@ -632,8 +632,8 @@ struct drm_mode_connector_set_property { #define DRM_MODE_OBJECT_ANY 0
struct drm_mode_obj_get_properties { - __u64 props_ptr; - __u64 prop_values_ptr; + __kernel_uintptr_t props_ptr; + __kernel_uintptr_t prop_values_ptr; __u32 count_props; __u32 obj_id; __u32 obj_type; @@ -649,7 +649,7 @@ struct drm_mode_obj_set_property { struct drm_mode_get_blob { __u32 blob_id; __u32 length; - __u64 data; + __kernel_uintptr_t data; };
struct drm_mode_fb_cmd { diff --git a/tools/include/uapi/drm/drm.h b/tools/include/uapi/drm/drm.h index de723566c5ae..5efb944a89c5 100644 --- a/tools/include/uapi/drm/drm.h +++ b/tools/include/uapi/drm/drm.h @@ -1289,7 +1289,7 @@ struct drm_event_vblank { */ struct drm_event_crtc_sequence { struct drm_event base; - __u64 user_data; + __u64 user_data; __s64 time_ns; __u64 sequence; };
linux-morello@op-lists.linaro.org
-
Carsten Haitzler -
carsten.haitzler＠foss.arm.com
-
Kevin Brodsky