The "mmc: Support capabilities in MMC_IOC_{MULTI_}CMD ioctls" patch previously hard-coded the ioctl command value in order to maintain compatibility with non-PCuABI ioctl handlers such as those found in block.c. This deviates from the standard approach which is to always define the ioctl command value in terms of the struct size (which may change), instead providing any fixes in the kernel via compat ioctl handlers.
This patch therefore aligns the MMC ioctl handlers in block.c with the standard approach. Undoing the hard-coding in the header leaves the ioctl command value unchanged in all situations except in PCuABI where the structs sizes, and hence the ioctl command values, are now larger. (The ioctl value remains unchanged in compat64 as well).
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com --- drivers/mmc/core/block.c | 62 ++++++++++++++++++++++++++++++++-- include/uapi/linux/mmc/ioctl.h | 21 +++++------- 2 files changed, 69 insertions(+), 14 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 17a1d24bdaed..d9b50bbd54ee 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -76,6 +76,15 @@ MODULE_ALIAS("mmc:block"); #define MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF0000) >> 16) #define MMC_EXTRACT_VALUE_FROM_ARG(x) ((x & 0x0000FF00) >> 8)
+/* + * The compat_mmc_ioc_{multi_}cmd structs have different sizes in PCuABI and in + * compat64, which affects the expected MMC_IOC_{MULTI_}CMD ioctl values. The + * following ioctl values reflect the sizes of the compat64 structs defined in + * this file. + */ +#define MMC_IOC_CMD_COMPAT _IOWR(MMC_BLOCK_MAJOR, 0, struct compat_mmc_ioc_cmd) +#define MMC_IOC_MULTI_CMD_COMPAT _IOWR(MMC_BLOCK_MAJOR, 1, struct compat_mmc_ioc_multi_cmd) + static DEFINE_MUTEX(block_mutex);
/* @@ -891,7 +900,37 @@ static int mmc_blk_ioctl(struct block_device *bdev, blk_mode_t mode, static int mmc_blk_compat_ioctl(struct block_device *bdev, blk_mode_t mode, unsigned int cmd, unsigned long arg) { - return mmc_blk_ioctl(bdev, mode, cmd, (user_uintptr_t) compat_ptr(arg)); + struct mmc_blk_data *md; + int ret; + + switch (cmd) { + case MMC_IOC_CMD_COMPAT: + ret = mmc_blk_check_blkdev(bdev); + if (ret) + return ret; + md = mmc_blk_get(bdev->bd_disk); + if (!md) + return -EINVAL; + ret = mmc_blk_ioctl_cmd(md, + (struct mmc_ioc_cmd __user *)arg, + NULL); + mmc_blk_put(md); + return ret; + case MMC_IOC_MULTI_CMD_COMPAT: + ret = mmc_blk_check_blkdev(bdev); + if (ret) + return ret; + md = mmc_blk_get(bdev->bd_disk); + if (!md) + return -EINVAL; + ret = mmc_blk_ioctl_multi_cmd(md, + (struct mmc_ioc_multi_cmd __user *)arg, + NULL); + mmc_blk_put(md); + return ret; + default: + return -EINVAL; + } } #endif
@@ -2700,7 +2739,26 @@ static long mmc_rpmb_ioctl(struct file *filp, unsigned int cmd, static long mmc_rpmb_ioctl_compat(struct file *filp, unsigned int cmd, unsigned long arg) { - return mmc_rpmb_ioctl(filp, cmd, (user_uintptr_t)compat_ptr(arg)); + struct mmc_rpmb_data *rpmb = filp->private_data; + int ret; + + switch (cmd) { + case MMC_IOC_CMD_COMPAT: + ret = mmc_blk_ioctl_cmd(rpmb->md, + (struct mmc_ioc_cmd __user *)arg, + rpmb); + break; + case MMC_IOC_MULTI_CMD_COMPAT: + ret = mmc_blk_ioctl_multi_cmd(rpmb->md, + (struct mmc_ioc_multi_cmd __user *)arg, + rpmb); + break; + default: + ret = -EINVAL; + break; + } + + return ret; } #endif
diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h index 9d34bfc49198..e99d932268c6 100644 --- a/include/uapi/linux/mmc/ioctl.h +++ b/include/uapi/linux/mmc/ioctl.h @@ -67,26 +67,23 @@ struct mmc_ioc_multi_cmd { /* * The size of struct mmc_ioc_cmd changes in PCuABI due to the use of * __kernel_uintptr_t, which in turn modifies the command value for this - * ioctl. It is therefore necessary to hard-code the value for the ioctl - * using the size of the struct that is expected in any ABI other than - * PCuABI, to ensure that the value for MMC_IOC_CMD is unchanged. The - * original definition is as follows: - * - * #define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd) + * ioctl. As this command value is not hard-coded to represent the original + * size of the struct, any ioctl handlers in compat64 must handle this change + * in values instead. */ -#define MMC_IOC_CMD _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 72) +#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd) + /* * MMC_IOC_MULTI_CMD: Used to send an array of MMC commands described by * the structure mmc_ioc_multi_cmd. The MMC driver will issue all * commands in array in sequence to card. * * Note: the size of struct mmc_ioc_multi_cmd changes in PCuABI, due to the - * increased alignment of struct mmc_ioc_cmd. For that reason, its value needs - * to be hard-coded too. Original definition: - * - * #define MMC_IOC_MULTI_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct mmc_ioc_multi_cmd) + * increased alignment of struct mmc_ioc_cmd. Similarly to MMC_IOC_CMD, any + * ioctl handlers must account for this change when running in compat64. */ -#define MMC_IOC_MULTI_CMD _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 1, 8) +#define MMC_IOC_MULTI_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct mmc_ioc_multi_cmd) + /* * Since this ioctl is only meant to enhance (and not replace) normal access * to the mmc bus device, an upper data transfer limit of MMC_IOC_MAX_BYTES
On 11/04/2024 21:38, Akram Ahmad wrote:
The "mmc: Support capabilities in MMC_IOC_{MULTI_}CMD ioctls" patch previously hard-coded the ioctl command value in order to maintain compatibility with non-PCuABI ioctl handlers such as those found in block.c. This deviates from the standard approach which is to always define the ioctl command value in terms of the struct size (which may change), instead providing any fixes in the kernel via compat ioctl handlers.
This patch therefore aligns the MMC ioctl handlers in block.c with the standard approach. Undoing the hard-coding in the header leaves the ioctl command value unchanged in all situations except in PCuABI where the structs sizes, and hence the ioctl command values, are now larger. (The ioctl value remains unchanged in compat64 as well).
Much clearer now, thanks!
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com
drivers/mmc/core/block.c | 62 ++++++++++++++++++++++++++++++++-- include/uapi/linux/mmc/ioctl.h | 21 +++++------- 2 files changed, 69 insertions(+), 14 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 17a1d24bdaed..d9b50bbd54ee 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -76,6 +76,15 @@ MODULE_ALIAS("mmc:block"); #define MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF0000) >> 16) #define MMC_EXTRACT_VALUE_FROM_ARG(x) ((x & 0x0000FF00) >> 8) +/*
- The compat_mmc_ioc_{multi_}cmd structs have different sizes in PCuABI and in
- compat64, which affects the expected MMC_IOC_{MULTI_}CMD ioctl values. The
- following ioctl values reflect the sizes of the compat64 structs defined in
- this file.
- */
+#define MMC_IOC_CMD_COMPAT _IOWR(MMC_BLOCK_MAJOR, 0, struct compat_mmc_ioc_cmd) +#define MMC_IOC_MULTI_CMD_COMPAT _IOWR(MMC_BLOCK_MAJOR, 1, struct compat_mmc_ioc_multi_cmd)
static DEFINE_MUTEX(block_mutex); /* @@ -891,7 +900,37 @@ static int mmc_blk_ioctl(struct block_device *bdev, blk_mode_t mode, static int mmc_blk_compat_ioctl(struct block_device *bdev, blk_mode_t mode, unsigned int cmd, unsigned long arg) {
- return mmc_blk_ioctl(bdev, mode, cmd, (user_uintptr_t) compat_ptr(arg));
- struct mmc_blk_data *md;
- int ret;
- switch (cmd) {
- case MMC_IOC_CMD_COMPAT:
ret = mmc_blk_check_blkdev(bdev);if (ret)return ret;md = mmc_blk_get(bdev->bd_disk);if (!md)return -EINVAL;ret = mmc_blk_ioctl_cmd(md,(struct mmc_ioc_cmd __user *)arg,
This cast is invalid, arg being an unsigned long in this case. I'm surprised the compiler doesn't warn about this.
For both functions, I'd suggest using something like:
void __user *uarg = compat_ptr(arg);
in the prologue. This avoids the need for a cast when calling the actual handler with uarg (void * is implicitly convertible to any pointer type), which is not just shorter but also avoids this strange cast to a pointer to a native struct, despite the pointer being actually to a compat struct in this case.
NULL);mmc_blk_put(md);return ret;- case MMC_IOC_MULTI_CMD_COMPAT:
ret = mmc_blk_check_blkdev(bdev);if (ret)return ret;md = mmc_blk_get(bdev->bd_disk);if (!md)return -EINVAL;ret = mmc_blk_ioctl_multi_cmd(md,(struct mmc_ioc_multi_cmd __user *)arg,NULL);mmc_blk_put(md);return ret;- default:
return -EINVAL;- }
} #endif @@ -2700,7 +2739,26 @@ static long mmc_rpmb_ioctl(struct file *filp, unsigned int cmd, static long mmc_rpmb_ioctl_compat(struct file *filp, unsigned int cmd, unsigned long arg) {
- return mmc_rpmb_ioctl(filp, cmd, (user_uintptr_t)compat_ptr(arg));
- struct mmc_rpmb_data *rpmb = filp->private_data;
- int ret;
- switch (cmd) {
- case MMC_IOC_CMD_COMPAT:
ret = mmc_blk_ioctl_cmd(rpmb->md,(struct mmc_ioc_cmd __user *)arg,rpmb);break;- case MMC_IOC_MULTI_CMD_COMPAT:
ret = mmc_blk_ioctl_multi_cmd(rpmb->md,(struct mmc_ioc_multi_cmd __user *)arg,rpmb);break;- default:
ret = -EINVAL;break;- }
- return ret;
} #endif diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h index 9d34bfc49198..e99d932268c6 100644 --- a/include/uapi/linux/mmc/ioctl.h +++ b/include/uapi/linux/mmc/ioctl.h @@ -67,26 +67,23 @@ struct mmc_ioc_multi_cmd { /*
- The size of struct mmc_ioc_cmd changes in PCuABI due to the use of
- __kernel_uintptr_t, which in turn modifies the command value for this
- ioctl. It is therefore necessary to hard-code the value for the ioctl
- using the size of the struct that is expected in any ABI other than
- PCuABI, to ensure that the value for MMC_IOC_CMD is unchanged. The
- original definition is as follows:
- #define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
- ioctl. As this command value is not hard-coded to represent the original
- size of the struct, any ioctl handlers in compat64 must handle this change
- in values instead.
I don't think there's much point in keeping these comments now that we're doing the normal (and expected) thing. We could revert that header back to how it was before "mmc: Support capabilities in MMC_IOC_{MULTI_}CMD ioctls".
Kevin
*/ -#define MMC_IOC_CMD _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 72) +#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
/*
- MMC_IOC_MULTI_CMD: Used to send an array of MMC commands described by
- the structure mmc_ioc_multi_cmd. The MMC driver will issue all
- commands in array in sequence to card.
- Note: the size of struct mmc_ioc_multi_cmd changes in PCuABI, due to the
- increased alignment of struct mmc_ioc_cmd. For that reason, its value needs
- to be hard-coded too. Original definition:
- #define MMC_IOC_MULTI_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct mmc_ioc_multi_cmd)
- increased alignment of struct mmc_ioc_cmd. Similarly to MMC_IOC_CMD, any
*/
- ioctl handlers must account for this change when running in compat64.
-#define MMC_IOC_MULTI_CMD _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 1, 8) +#define MMC_IOC_MULTI_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct mmc_ioc_multi_cmd)
/*
- Since this ioctl is only meant to enhance (and not replace) normal access
- to the mmc bus device, an upper data transfer limit of MMC_IOC_MAX_BYTES
On 12/04/2024 13:26, Kevin Brodsky wrote:
This cast is invalid, arg being an unsigned long in this case. I'm surprised the compiler doesn't warn about this.
For both functions, I'd suggest using something like:
void __user *uarg = compat_ptr(arg);
in the prologue. This avoids the need for a cast when calling the actual handler with uarg (void * is implicitly convertible to any pointer type), which is not just shorter but also avoids this strange cast to a pointer to a native struct, despite the pointer being actually to a compat struct in this case.
I thought there was something fishy here! Do you know why the compiler would ignore this, could it have anything to do with how I've got the build configured (specifically the #ifdef CONFIG_COMPAT)?
Many thanks for the feedback as always,
Akram
On 15/04/2024 14:35, Akram Ahmad wrote:
On 12/04/2024 13:26, Kevin Brodsky wrote:
This cast is invalid, arg being an unsigned long in this case. I'm surprised the compiler doesn't warn about this.
For both functions, I'd suggest using something like:
void __user *uarg = compat_ptr(arg);
in the prologue. This avoids the need for a cast when calling the actual handler with uarg (void * is implicitly convertible to any pointer type), which is not just shorter but also avoids this strange cast to a pointer to a native struct, despite the pointer being actually to a compat struct in this case.
I thought there was something fishy here! Do you know why the compiler would ignore this, could it have anything to do with how I've got the build configured (specifically the #ifdef CONFIG_COMPAT)?
This is actually an old issue with CHERI-LLVM [1], somehow it still gets me...
Many thanks for the feedback as always,
Most welcome!
Kevin
Akram
linux-morello@op-lists.linaro.org
-
Akram Ahmad -
Kevin Brodsky