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.
Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com
drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_ioctl.c | 20 ++++++++------------ 2 files changed, 9 insertions(+), 13 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..950a83828cc9 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -469,9 +469,9 @@ EXPORT_SYMBOL(drm_invalid_op); /*
- Copy and IOCTL return string to user space
*/ -static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value) +static int drm_copy_field(char __user *buf, size_t __user *buf_len, const char *value)
buf_len is not a user pointer. Clang has this very unfortunate bug in hybrid where a integer pointer can be implicitly converted to a capability... that's probably why this went unnoticed.
Ummm... oh yeah... i may have gone overboard :) it happened to work and nothing pointed me to it. the buf is userspace - buf_len is not provided as such by userspace...
{
- size_t len;
- size_t len, len_orig;
/* don't attempt to copy a NULL pointer */ if (WARN_ONCE(!value, "BUG: the value to copy was not set!")) { @@ -480,13 +480,13 @@ static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value) } /* don't overflow userbuf */
- len = strlen(value);
- len = len_orig = strlen(value); if (len > *buf_len) len = *buf_len;
/* let userspace know exact length of driver value (which could be * larger than the userspace-supplied buffer) */
- *buf_len = strlen(value);
- *buf_len = len_orig;
That looks like a nice improvement (removing an unnecessary call to strlen()) but isn't it unrelated to what this patch does overall?
Good point - it snuck in with it... I can split this out.
Kevin
/* finally, try filling in the userbuf */ if (len && buf) @@ -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",