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...