A previous patch which introduced support for capabilities in the MMC_IOC_{MULTI_}CMD ioctls also hard-coded the ioctl values to maintain compatibility with non-PCuABI ioctl handlers such as those found in block.c.
This patch therefore removes the hard-coding of these ioctl values, and enforces compatibility by accounting for the change in values if the ioctl handler is running in compat64.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com --- drivers/mmc/core/block.c | 21 +++++++++++++++++++++ include/uapi/linux/mmc/ioctl.h | 21 +++++++++------------ 2 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 17a1d24bdaed..acb7f3a9b31a 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 COMPAT64_MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct compat_mmc_ioc_cmd) +#define COMPAT64_MMC_IOC_MULTI_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct compat_mmc_ioc_multi_cmd) + static DEFINE_MUTEX(block_mutex);
/* @@ -858,6 +867,9 @@ static int mmc_blk_ioctl(struct block_device *bdev, blk_mode_t mode, int ret;
switch (cmd) { + case COMPAT64_MMC_IOC_CMD: + if (!in_compat64_syscall()) + return -EINVAL; case MMC_IOC_CMD: ret = mmc_blk_check_blkdev(bdev); if (ret) @@ -870,6 +882,9 @@ static int mmc_blk_ioctl(struct block_device *bdev, blk_mode_t mode, NULL); mmc_blk_put(md); return ret; + case COMPAT64_MMC_IOC_MULTI_CMD: + if (!in_compat64_syscall()) + return -EINVAL; case MMC_IOC_MULTI_CMD: ret = mmc_blk_check_blkdev(bdev); if (ret) @@ -2678,11 +2693,17 @@ static long mmc_rpmb_ioctl(struct file *filp, unsigned int cmd, int ret;
switch (cmd) { + case COMPAT64_MMC_IOC_CMD: + if (!in_compat64_syscall()) + return -EINVAL; case MMC_IOC_CMD: ret = mmc_blk_ioctl_cmd(rpmb->md, (struct mmc_ioc_cmd __user *)arg, rpmb); break; + case COMPAT64_MMC_IOC_MULTI_CMD: + if (!in_compat64_syscall()) + return -EINVAL; case MMC_IOC_MULTI_CMD: ret = mmc_blk_ioctl_multi_cmd(rpmb->md, (struct mmc_ioc_multi_cmd __user *)arg, 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 28/03/2024 16:28, Akram Ahmad wrote:
A previous patch which introduced support for capabilities in the MMC_IOC_{MULTI_}CMD ioctls also hard-coded the ioctl values to maintain compatibility with non-PCuABI ioctl handlers such as those found in block.c.
This patch therefore removes the hard-coding of these ioctl values, and enforces compatibility by accounting for the change in values if the ioctl handler is running in compat64.
A few things to clarify here: - Which commit you are referring to (without mentioning the hash as usual for our own commits). - Exactly what is the effect of removing the hardcoding in the uapi header, i.e. the ioctl command numbers remain unchanged in all situations (including compat64) *except* PCuABI, where they are now defined in terms of the (larger) struct sizes in PCuABI. - Why we're doing this (essentially aligning with the standard approach where command numbers are defined in terms of the struct size and the kernel handles the difference in command number in compat).
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com
drivers/mmc/core/block.c | 21 +++++++++++++++++++++ include/uapi/linux/mmc/ioctl.h | 21 +++++++++------------ 2 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 17a1d24bdaed..acb7f3a9b31a 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 COMPAT64_MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct compat_mmc_ioc_cmd) +#define COMPAT64_MMC_IOC_MULTI_CMD _IOWR(MMC_BLOCK_MAJOR, 1, struct compat_mmc_ioc_multi_cmd)
To be consistent with the way these are generally named, let's make COMPAT a suffix instead of a prefix.
static DEFINE_MUTEX(block_mutex); /* @@ -858,6 +867,9 @@ static int mmc_blk_ioctl(struct block_device *bdev, blk_mode_t mode, int ret; switch (cmd) {
- case COMPAT64_MMC_IOC_CMD:
if (!in_compat64_syscall())
That will break any configuration other than PCuABI + compat64, because MMC_IOC_CMD and COMPAT64_MMC_IOC_CMD then end up being equal.
In fact it looks like we can do all this the simple way without duplicating too much code. We already have a compat handler (mmc_blk_compat_ioctl()), and mmc_blk_ioctl() doesn't have much logic in it. So what we can simply do is to copy the whole body of the native handler into the compat handler, instead of calling the native handler from there, and replace the values in the switch with the compat command numbers. It does create some duplication, but the big advantage is that there is no need for additional compat checks: we use the native command values in native, and the compat values in compat, and that's it. In that case it makes more sense to call the values "COMPAT" instead of "COMPAT64", because they will always be used in compat.
The same logic should work for mmc_rpmb_ioctl().
Kevin
return -EINVAL;@@ -870,6 +882,9 @@ static int mmc_blk_ioctl(struct block_device *bdev, blk_mode_t mode, NULL); mmc_blk_put(md); return ret;
- case COMPAT64_MMC_IOC_MULTI_CMD:
if (!in_compat64_syscall())return -EINVAL;@@ -2678,11 +2693,17 @@ static long mmc_rpmb_ioctl(struct file *filp, unsigned int cmd, int ret; switch (cmd) {
- case COMPAT64_MMC_IOC_CMD:
if (!in_compat64_syscall())return -EINVAL;- case COMPAT64_MMC_IOC_MULTI_CMD:
if (!in_compat64_syscall())return -EINVAL;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
linux-morello@op-lists.linaro.org
-
Akram Ahmad -
Kevin Brodsky