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