On 11/04/2024 14:19, Carsten Haitzler wrote:
On 4/9/24 3:58 PM, Kevin Brodsky wrote:
Typo in title: "trasnport"
Or maybe simply say "to preserve capabilities"?
On 05/04/2024 12:20, carsten.haitzler@foss.arm.com wrote:
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. Also don't use stack as the kernel side buffer (optimization for smaller payloads) doesn't work.
Any idea why it doesn't work?
I don't actually know - I was just getting invalid capabilities as long as it was copying onto the stack buffer (128 bytes or less). I discovered moving it to allocated memory and it worked. It was a trivial change so I moved on.
Actually I know why, having had another look. stack_kdata is an array of char, so its alignment is 1. Considering the other local variables, it's very likely that it will not end up being 16-byte aligned, and as a result it cannot be used as storage for a struct with capabilities. That could be fixed by adding some alignment attribute. Getting rid of it is probably ok too, let's just explain why.
Kevin
[...]
@@ -821,7 +821,6 @@ 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 *kdata = NULL; unsigned int in_size, out_size, drv_size, ksize; bool is_driver_ioctl; @@ -874,9 +873,7 @@ long drm_ioctl(struct file *filp, goto err_i1; } - if (ksize <= sizeof(stack_kdata)) { - kdata = stack_kdata; - } else { + if (ksize > 0) { kdata = kmalloc(ksize, GFP_KERNEL); if (!kdata) { retcode = -ENOMEM; @@ -884,16 +881,15 @@ 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; }
if (ksize > in_size) 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: @@ -904,7 +900,7 @@ long drm_ioctl(struct file *filp, (long)old_encode_dev(file_priv->minor->kdev->devt), file_priv->authenticated, cmd, nr); - if (kdata != stack_kdata) + if (kdata != NULL) kfree(kdata); if (retcode) drm_dbg_core(dev, "comm="%s", pid=%d, ret=%d\n",