[linux-morello] Re: [PATCH v2 1/3] drm: Fix copy to/from user so that caps trasnport in the region

On 12/04/2024 18:06, carsten.haitzler@foss.arm.com wrote:

From: Carsten Haitzler carsten.haitzler@arm.com

Typo in title: "trasnport"

Or maybe simply say "to preserve capabilities"?

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 icotls (128 bytes or less) is aligned to 16 bytes.

s/icotls/ioctls/

This possibly a bit of wasted space on the stack but north it for

"This is", s/north/worth/

simplicity of code.

We could use alignof(__kernel_uintptr_t) instead of hardcoding 16, but I don't mind either way. Could be worth considering a patch for upstream though, especially since it could break all architectures that don't like unaligned loads/stores (surely there are some on which DRM is used?).

Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com

drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_ioctl.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 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..72e83d87936d 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -469,7 +469,7 @@ 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)

We don't want this right (as discussed in v1)?

Kevin

{ size_t len; @@ -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(16))); 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: