[linux-morello] Re: [PATCH] drm/compat: Fix types for compat to allow for non-32bit

On 2/2/23 08:12, Kevin Brodsky wrote:

On 30/01/2023 14:50, carsten.haitzler@foss.arm.com wrote:

...

From: Carsten Haitzler carsten.haitzler@foss.arm.com

Careful, it looks like you ended up committing with a different email address from the one you S-o-b with.

As they are both me... :) the problem is I had to use @foss because of complaints about arm adding its disclaimer blurbs to my @arm.com mails - si i send from my @foss.arm.com to avoid that. So far upstreams have not cared about the mix.

...

DRM compat code totally assumes 32bit is compat. This is not always

Nit: rather "compat is 32-bit", native can also be 32-bit :)

That is true. I guess from the morello context it is... but in the event of native 32bit we have no comapt at all so it's moot. :) but the drm compat code actually is literally all 32bit only. It totally assumes that.

...

the case. This allows for compat to be defined universally to typedefed types like compat_ulong_t and so on. This converts some of the drm compat handling for ioctl's to use these types.

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

drivers/gpu/drm/drm_ioc32.c | 209 ++++++++++++++++++------------------ include/drm/drm_ioctl.h | 2 +- 2 files changed, 106 insertions(+), 105 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c index 5d82891c3222..13e7d25dcbae 100644 --- a/drivers/gpu/drm/drm_ioc32.c +++ b/drivers/gpu/drm/drm_ioc32.c @@ -38,21 +38,21 @@ #include "drm_internal.h" #include "drm_legacy.h" -#define DRM_IOCTL_VERSION32 DRM_IOWR(0x00, drm_version32_t) -#define DRM_IOCTL_GET_UNIQUE32 DRM_IOWR(0x01, drm_unique32_t) +#define DRM_IOCTL_VERSION_COMPAT DRM_IOWR(0x00, drm_version_compat_t) +#define DRM_IOCTL_GET_UNIQUE_COMPAT DRM_IOWR(0x01, drm_unique_compat_t)

On balance, I don't think this partial 32 -> compat renaming is really helping. A large number of structs / constants in this file remain suffixed with 32, making it look like some of them are 32-bit-specific and some are not. If we really want to go for renaming here, we should rename everything in this file, and even the file name itself. That would be extremely noisy, which is rather undesirable for a patch we will keep rebasing. AFAICT the functional changes in this patch are fairly small, so I would stick to those and skip the renaming.

If I do that then I start fixing legacy (which is not needed) and x86 only 32bit compat code (CONFIG_X86). If you start doing this path, the patch starts getting a lot bigger as I have to change various other files in the tree. I start having to make changes to struct file (or was it struct file_operations ... I can't remember .. something along these lines). I started walking down this path and went "oh dear" as I noticed I'd have to touch 100's of files and then unwound that work and ketp it to the necessary pieces. The hybrid nature of this kis a result of keeping this concise and not sprawling as well as sticking to what is being actually tested.

...

#define DRM_IOCTL_GET_MAP32 DRM_IOWR(0x04, drm_map32_t) -#define DRM_IOCTL_GET_CLIENT32 DRM_IOWR(0x05, drm_client32_t) -#define DRM_IOCTL_GET_STATS32 DRM_IOR( 0x06, drm_stats32_t) +#define DRM_IOCTL_GET_CLIENT_COMPAT DRM_IOWR(0x05, drm_client_compat_t) +#define DRM_IOCTL_GET_STATS_COMPAT DRM_IOR( 0x06, drm_stats_compat_t) -#define DRM_IOCTL_SET_UNIQUE32 DRM_IOW( 0x10, drm_unique32_t) -#define DRM_IOCTL_ADD_MAP32 DRM_IOWR(0x15, drm_map32_t) +#define DRM_IOCTL_SET_UNIQUE_COMPAT DRM_IOW( 0x10, drm_unique_compat_t) +#define DRM_IOCTL_ADD_MAP_COMPAT DRM_IOWR(0x15, drm_map_compat_t) #define DRM_IOCTL_ADD_BUFS32 DRM_IOWR(0x16, drm_buf_desc32_t) #define DRM_IOCTL_MARK_BUFS32 DRM_IOW( 0x17, drm_buf_desc32_t) #define DRM_IOCTL_INFO_BUFS32 DRM_IOWR(0x18, drm_buf_info32_t) #define DRM_IOCTL_MAP_BUFS32 DRM_IOWR(0x19, drm_buf_map32_t) #define DRM_IOCTL_FREE_BUFS32 DRM_IOW( 0x1a, drm_buf_free32_t) -#define DRM_IOCTL_RM_MAP32 DRM_IOW( 0x1b, drm_map32_t) +#define DRM_IOCTL_RM_MAP_COMPAT DRM_IOW( 0x1b, drm_map_compat_t) #define DRM_IOCTL_SET_SAREA_CTX32 DRM_IOW( 0x1c, drm_ctx_priv_map32_t) #define DRM_IOCTL_GET_SAREA_CTX32 DRM_IOWR(0x1d, drm_ctx_priv_map32_t) @@ -72,92 +72,92 @@ #define DRM_IOCTL_UPDATE_DRAW32 DRM_IOW( 0x3f, drm_update_draw32_t) -#define DRM_IOCTL_WAIT_VBLANK32 DRM_IOWR(0x3a, drm_wait_vblank32_t) +#define DRM_IOCTL_WAIT_VBLANK_COMPAT DRM_IOWR(0x3a, drm_wait_vblank_compat_t) #define DRM_IOCTL_MODE_ADDFB232 DRM_IOWR(0xb8, drm_mode_fb_cmd232_t) -typedef struct drm_version_32 { +typedef struct drm_version_compat { int version_major; /* Major version */ int version_minor; /* Minor version */ int version_patchlevel; /* Patch level */

• u32 name_len; /* Length of name buffer */
• u32 name; /* Name of driver */
• u32 date_len; /* Length of date buffer */
• u32 date; /* User-space buffer to hold date */
• u32 desc_len; /* Length of desc buffer */
• u32 desc; /* User-space buffer to hold desc */

-} drm_version32_t;

• compat_size_t name_len; /* Length of name buffer */
• compat_uptr_t name; /* Name of driver */
• compat_size_t date_len; /* Length of date buffer */
• compat_uptr_t date; /* User-space buffer to hold date */
• compat_size_t desc_len; /* Length of desc buffer */
• compat_uptr_t desc; /* User-space buffer to hold desc */

+} drm_version_compat_t; static int compat_drm_version(struct file *file, unsigned int cmd,

• 	      unsigned long arg)
  

• 	      user_uintptr_t arg)
  

  {

• drm_version32_t v32;

• drm_version_compat_t v_compat; struct drm_version v; int err;

• if (copy_from_user(&v32, (void __user *)arg, sizeof(v32)))

• if (copy_from_user(&v_compat, (void __user *)arg, sizeof(v_compat))) return -EFAULT;

memset(&v, 0, sizeof(v)); v = (struct drm_version) {

• .name_len = v32.name_len,
  

• .name = compat_ptr(v32.name),
  

• .date_len = v32.date_len,
  

• .date = compat_ptr(v32.date),
  

• .desc_len = v32.desc_len,
  

• .desc = compat_ptr(v32.desc),
  

• .name_len = v_compat.name_len,
  

• .name = compat_ptr(v_compat.name),
  

• .date_len = v_compat.date_len,
  

• .date = compat_ptr(v_compat.date),
  

• .desc_len = v_compat.desc_len,
  

• .desc = compat_ptr(v_compat.desc),
  

  }; err = drm_ioctl_kernel(file, drm_version, &v, DRM_RENDER_ALLOW); if (err) return err;

• v32.version_major = v.version_major;
• v32.version_minor = v.version_minor;
• v32.version_patchlevel = v.version_patchlevel;
• v32.name_len = v.name_len;
• v32.date_len = v.date_len;
• v32.desc_len = v.desc_len;
• if (copy_to_user((void __user *)arg, &v32, sizeof(v32)))

• v_compat.version_major = v.version_major;
• v_compat.version_minor = v.version_minor;
• v_compat.version_patchlevel = v.version_patchlevel;
• v_compat.name_len = v.name_len;
• v_compat.date_len = v.date_len;
• v_compat.desc_len = v.desc_len;
• if (copy_to_user((void __user *)arg, &v_compat, sizeof(v_compat))) return -EFAULT; return 0; }

-typedef struct drm_unique32 {

• u32 unique_len; /* Length of unique */
• u32 unique; /* Unique name for driver instantiation */

-} drm_unique32_t; +typedef struct drm_unique_compat {

• compat_size_t unique_len; /* Length of unique */
• compat_uptr_t unique; /* Unique name for driver instantiation */

+} drm_unique_compat_t; static int compat_drm_getunique(struct file *file, unsigned int cmd,

• 		unsigned long arg)
  

• 		user_uintptr_t arg)
  

  {

• drm_unique32_t uq32;

• drm_unique_compat_t uq_compat; struct drm_unique uq; int err;

• if (copy_from_user(&uq32, (void __user *)arg, sizeof(uq32)))

• if (copy_from_user(&uq_compat, (void __user *)arg, sizeof(uq_compat))) return -EFAULT;

memset(&uq, 0, sizeof(uq)); uq = (struct drm_unique){

• .unique_len = uq32.unique_len,
  

• .unique = compat_ptr(uq32.unique),
  

• .unique_len = uq_compat.unique_len,
  

• .unique = compat_ptr(uq_compat.unique),
  

  };

err = drm_ioctl_kernel(file, drm_getunique, &uq, 0); if (err) return err;

• uq32.unique_len = uq.unique_len;
• if (copy_to_user((void __user *)arg, &uq32, sizeof(uq32)))

• uq_compat.unique_len = uq.unique_len;
• if (copy_to_user((void __user *)arg, &uq_compat, sizeof(uq_compat))) return -EFAULT; return 0; }

static int compat_drm_setunique(struct file *file, unsigned int cmd,

• 		unsigned long arg)
  

• 		user_uintptr_t arg)
  

  { /* it's dead */ return -EINVAL;

@@ -167,14 +167,14 @@ static int compat_drm_setunique(struct file *file, unsigned int cmd, typedef struct drm_map32 { u32 offset; /* Requested physical address (0 for SAREA) */ u32 size; /* Requested physical size (bytes) */

• enum drm_map_type type; /* Type of memory to map */

• enum drm_map_type type; /* Type of memory to map */ enum drm_map_flags flags; /* Flags */ u32 handle; /* User-space: "Handle" to pass to mmap() */

• int mtrr; /* MTRR slot used */

• int mtrr; /* MTRR slot used */

Spurious change (above as well)?

...

} drm_map32_t; static int compat_drm_getmap(struct file *file, unsigned int cmd,

• 	     unsigned long arg)
  

• 	     user_uintptr_t arg)
  

  { drm_map32_t __user *argp = (void __user *)arg; drm_map32_t m32;

@@ -193,7 +193,7 @@ static int compat_drm_getmap(struct file *file, unsigned int cmd, m32.size = map.size; m32.type = map.type; m32.flags = map.flags;

• m32.handle = ptr_to_compat((void __user *)map.handle);

• m32.handle = ptr_to32((void __user *)map.handle);

Ehm, ptr_to32(), that's a new one :) I guess we're not enabling DRM_LEGACY so this code is just not built, which is why this went unnoticed?

Good catch - this was part of me unwinding my work in legacy then on to x86 code... oops. this is where it started to touch loads of files and i had to undo it.. as it wasnt compiled due to config - i didnt notice :) will remove.

...

m32.mtrr = map.mtrr; if (copy_to_user(argp, &m32, sizeof(m32))) return -EFAULT; @@ -202,7 +202,7 @@ static int compat_drm_getmap(struct file *file, unsigned int cmd, } static int compat_drm_addmap(struct file *file, unsigned int cmd,

• 	     unsigned long arg)
  

• 	     user_uintptr_t arg)
  

  { drm_map32_t __user *argp = (void __user *)arg; drm_map32_t m32;

@@ -224,7 +224,7 @@ static int compat_drm_addmap(struct file *file, unsigned int cmd, m32.offset = map.offset; m32.mtrr = map.mtrr;

• m32.handle = ptr_to_compat((void __user *)map.handle);

• m32.handle = ptr_to32((void __user *)map.handle); if (map.handle != compat_ptr(m32.handle)) pr_err_ratelimited("compat_drm_addmap truncated handle %p for type %d offset %x\n", map.handle, m32.type, m32.offset);

@@ -236,11 +236,11 @@ static int compat_drm_addmap(struct file *file, unsigned int cmd, } static int compat_drm_rmmap(struct file *file, unsigned int cmd,

• 	    unsigned long arg)
  

• 	    user_uintptr_t arg)
  

  { drm_map32_t __user *argp = (void __user *)arg; struct drm_map map;

• u32 handle;

• compat_uptr_t handle;

same for all the above. they are part of me walking down the path - once I got enough down it it led to me touching a lot of files... I'll undo these.

Makes sense, except that drm_map32_t::handle is still a u32. Overall I'm not keen on making non-trivial changes to code we're not building, such as everything guarded by CONFIG_DRM_LEGACY here, as we can easily create as many issues as we're solving. Alternatively, would it be doable to enable DRM_LEGACY to test these changes? Simply building locally with DRM_LEGACY enabled would be a good enough test in this case I think.

Yeah - as above. we aren't building legacy or x86 - i shouldn't have had these in the patch.

...

if (get_user(handle, &argp->handle)) return -EFAULT; @@ -249,61 +249,61 @@ static int compat_drm_rmmap(struct file *file, unsigned int cmd, } #endif -typedef struct drm_client32 {

• int idx; /* Which client desired? */
• int auth; /* Is client authenticated? */
• u32 pid; /* Process ID */
• u32 uid; /* User ID */
• u32 magic; /* Magic */
• u32 iocs; /* Ioctl count */

-} drm_client32_t; +typedef struct drm_client_compat {

• int idx; /* Which client desired? */
• int auth; /* Is client authenticated? */
• compat_ulong_t pid; /* Process ID */
• compat_ulong_t uid; /* User ID */
• compat_ulong_t magic; /* Magic */
• compat_ulong_t iocs; /* Ioctl count */

+} drm_client_compat_t; static int compat_drm_getclient(struct file *file, unsigned int cmd,

• 		unsigned long arg)
  

• 		user_uintptr_t arg)
  

  {

• drm_client32_t c32;
• drm_client32_t __user *argp = (void __user *)arg;

• drm_client_compat_t c_compat;
• drm_client_compat_t __user *argp = (void __user *)arg; struct drm_client client; int err;

• if (copy_from_user(&c32, argp, sizeof(c32)))

• if (copy_from_user(&c_compat, argp, sizeof(c_compat))) return -EFAULT;

memset(&client, 0, sizeof(client));

• client.idx = c32.idx;

• client.idx = c_compat.idx;

err = drm_ioctl_kernel(file, drm_getclient, &client, 0); if (err) return err;

• c32.idx = client.idx;
• c32.auth = client.auth;
• c32.pid = client.pid;
• c32.uid = client.uid;
• c32.magic = client.magic;
• c32.iocs = client.iocs;

• c_compat.idx = client.idx;
• c_compat.auth = client.auth;
• c_compat.pid = client.pid;
• c_compat.uid = client.uid;
• c_compat.magic = client.magic;
• c_compat.iocs = client.iocs;

• if (copy_to_user(argp, &c32, sizeof(c32)))

• if (copy_to_user(argp, &c_compat, sizeof(c_compat))) return -EFAULT; return 0; }

-typedef struct drm_stats32 {

• u32 count;

+typedef struct drm_stats_compat {

• compat_ulong_t count; struct {

• u32 value;
  

• compat_ulong_t value;
  

  enum drm_stat_type type; } data[15];

-} drm_stats32_t; +} drm_stats_compat_t; static int compat_drm_getstats(struct file *file, unsigned int cmd,

• 	       unsigned long arg)
  

• 	       user_uintptr_t arg)
  

  {

• drm_stats32_t __user *argp = (void __user *)arg;

• drm_stats_compat_t __user *argp = (void __user *)arg;

/* getstats is defunct, just clear */

• if (clear_user(argp, sizeof(drm_stats32_t)))

• if (clear_user(argp, sizeof(drm_stats_compat_t))) return -EFAULT; return 0; }

@@ -820,47 +820,47 @@ static int compat_drm_update_draw(struct file *file, unsigned int cmd, } #endif -struct drm_wait_vblank_request32 { +struct drm_wait_vblank_request_compat { enum drm_vblank_seq_type type; unsigned int sequence;

• u32 signal;

• compat_ulong_t signal; };

-struct drm_wait_vblank_reply32 { +struct drm_wait_vblank_reply_compat { enum drm_vblank_seq_type type; unsigned int sequence; s32 tval_sec; s32 tval_usec; }; -typedef union drm_wait_vblank32 {

• struct drm_wait_vblank_request32 request;
• struct drm_wait_vblank_reply32 reply;

-} drm_wait_vblank32_t; +typedef union drm_wait_vblank_compat {

• struct drm_wait_vblank_request_compat request;
• struct drm_wait_vblank_reply_compat reply;

+} drm_wait_vblank_compat_t; static int compat_drm_wait_vblank(struct file *file, unsigned int cmd,

• 		  unsigned long arg)
  

• 		  user_uintptr_t arg)
  

  {

• drm_wait_vblank32_t __user *argp = (void __user *)arg;
• drm_wait_vblank32_t req32;

• drm_wait_vblank_compat_t __user *argp = (void __user *)arg;
• drm_wait_vblank_compat_t req_compat; union drm_wait_vblank req; int err;

• if (copy_from_user(&req32, argp, sizeof(req32)))

• if (copy_from_user(&req_compat, argp, sizeof(req_compat))) return -EFAULT;

memset(&req, 0, sizeof(req));

• req.request.type = req32.request.type;
• req.request.sequence = req32.request.sequence;
• req.request.signal = req32.request.signal;

• req.request.type = req_compat.request.type;
• req.request.sequence = req_compat.request.sequence;
• req.request.signal = req_compat.request.signal; err = drm_ioctl_kernel(file, drm_wait_vblank_ioctl, &req, DRM_UNLOCKED);

• req32.reply.type = req.reply.type;
• req32.reply.sequence = req.reply.sequence;
• req32.reply.tval_sec = req.reply.tval_sec;
• req32.reply.tval_usec = req.reply.tval_usec;
• if (copy_to_user(argp, &req32, sizeof(req32)))

• req_compat.reply.type = req.reply.type;
• req_compat.reply.sequence = req.reply.sequence;
• req_compat.reply.tval_sec = req.reply.tval_sec;
• req_compat.reply.tval_usec = req.reply.tval_usec;
• if (copy_to_user(argp, &req_compat, sizeof(req_compat))) return -EFAULT;

return err; @@ -911,15 +911,16 @@ static struct { drm_ioctl_compat_t *fn; char *name; } drm_compat_ioctls[] = { +#define DRM_IOCTL_COMPAT_DEF(n, f) [DRM_IOCTL_NR(n##_COMPAT)] = {.fn = f, .name = #n} #define DRM_IOCTL32_DEF(n, f) [DRM_IOCTL_NR(n##32)] = {.fn = f, .name = #n}

• DRM_IOCTL32_DEF(DRM_IOCTL_VERSION, compat_drm_version),
• DRM_IOCTL32_DEF(DRM_IOCTL_GET_UNIQUE, compat_drm_getunique),

• DRM_IOCTL_COMPAT_DEF(DRM_IOCTL_VERSION, compat_drm_version),
• DRM_IOCTL_COMPAT_DEF(DRM_IOCTL_GET_UNIQUE, compat_drm_getunique), #if IS_ENABLED(CONFIG_DRM_LEGACY) DRM_IOCTL32_DEF(DRM_IOCTL_GET_MAP, compat_drm_getmap), #endif

• DRM_IOCTL32_DEF(DRM_IOCTL_GET_CLIENT, compat_drm_getclient),
• DRM_IOCTL32_DEF(DRM_IOCTL_GET_STATS, compat_drm_getstats),
• DRM_IOCTL32_DEF(DRM_IOCTL_SET_UNIQUE, compat_drm_setunique),

• DRM_IOCTL_COMPAT_DEF(DRM_IOCTL_GET_CLIENT, compat_drm_getclient),
• DRM_IOCTL_COMPAT_DEF(DRM_IOCTL_GET_STATS, compat_drm_getstats),
• DRM_IOCTL_COMPAT_DEF(DRM_IOCTL_SET_UNIQUE, compat_drm_setunique), #if IS_ENABLED(CONFIG_DRM_LEGACY) DRM_IOCTL32_DEF(DRM_IOCTL_ADD_MAP, compat_drm_addmap), DRM_IOCTL32_DEF(DRM_IOCTL_ADD_BUFS, compat_drm_addbufs),

@@ -948,7 +949,7 @@ static struct { #if defined(CONFIG_X86) || defined(CONFIG_IA64) DRM_IOCTL32_DEF(DRM_IOCTL_UPDATE_DRAW, compat_drm_update_draw), #endif

• DRM_IOCTL32_DEF(DRM_IOCTL_WAIT_VBLANK, compat_drm_wait_vblank),

• DRM_IOCTL_COMPAT_DEF(DRM_IOCTL_WAIT_VBLANK, compat_drm_wait_vblank), #if defined(CONFIG_X86) || defined(CONFIG_IA64) DRM_IOCTL32_DEF(DRM_IOCTL_MODE_ADDFB2, compat_drm_mode_addfb2), #endif

diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h index 26d5c31aefa9..a7fd12d09268 100644 --- a/include/drm/drm_ioctl.h +++ b/include/drm/drm_ioctl.h @@ -65,7 +65,7 @@ typedef int drm_ioctl_t(struct drm_device *dev, void *data,

• structures and hence never need this.

*/ typedef int drm_ioctl_compat_t(struct file *filp, unsigned int cmd,

• 	       unsigned long arg);
  

• 	       user_uintptr_t arg);
  

I think this makes sense. However, this is not enough to ensure that the handlers are passed an appropriate capability in PCuABI: you need to create it explicitly with compat_ptr(). (This issue goes unnoticed as we're not actually performing uaccess using capabilities yet.) Fortunately things are simple here as arg always represents a pointer, so you can do the conversion unconditionally in drm_compat_ioctl(), like in this section [1] of the porting guide. Both drm_ioctl() and these compat handlers expect arg to be a valid user pointer, so both should be passed the pointer created by compat_ptr().

Yeah.. oops. Will fix. Missed that.

Kevin

[1] https://git.morello-project.org/morello/kernel/linux/-/blob/morello/master/D...

...

#define DRM_IOCTL_NR(n) _IOC_NR(n) #define DRM_IOCTL_TYPE(n) _IOC_TYPE(n)