Hi everyone,
This patch aims to enable purecap applications to make use of the MMC block driver by adding PCuABI support to the MMC_IOC_CMD and MMC_IOC_MULTI_CMD ioctls. This also includes compat64 support for the ioctls.
V6 -> V7: Clarify comments on ioctl numbering.
V5 -> V6: Fix for ioctl numbering based on struct size; minor corrections.
V4 -> V5: Improvement to readability and fix to user-space macro.
V3 -> V4: Minor corrections and improvements to readability of code. - Remove unnecessary explicit checks on capabilities. - Revert modification to struct mmc_ioc_multi_cmd. - Remove unnecessary parentheses.
V2 -> V3: - Implement support for capabilities in the ioctls. - Correct formatting errors in patches. - Correct formatting and syntax errors in code.
V1 -> V2: Various improvements to the code including: - Preference for native structs over (void __user *) - Complying with code styling guidelines - Improvement in code readability via removing unnecessary casts
GitLab Issue: https://git.morello-project.org/morello/kernel/linux/-/issues/51
Review branch: https://git.morello-project.org/arkamnite/linux/-/commits/morello%2Fmmc_v7
Many thanks, Akram
Akram Ahmad (2): mmc: Implement compat handling for struct mmc_ioc_{multi_}cmd mmc: Support capabilities in MMC_IOC_{MULTI_}CMD ioctls
drivers/mmc/core/block.c | 100 ++++++++++++++++++++++++++++++--- include/uapi/linux/mmc/ioctl.h | 19 ++++++- 2 files changed, 108 insertions(+), 11 deletions(-)
Introduce a compat version of the struct mmc_ioc_cmd and struct mmc_ioc_multi_cmd. Also implement helper functions which convert between the native and compat versions of these two structs.
A subsequent patch will change the structs to enable it to support new architectures. On such architectures, the current struct layout must still be supported for compat tasks.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com --- drivers/mmc/core/block.c | 96 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 90 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 2f8884e3d4c2..3333c4917028 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -101,6 +101,33 @@ struct mmc_blk_busy_data { u32 status; };
+static inline bool in_compat64(void) +{ + return IS_ENABLED(CONFIG_COMPAT64) && in_compat_syscall(); +} + +struct compat_mmc_ioc_cmd { + int write_flag; + int is_acmd; + __u32 opcode; + __u32 arg; + __u32 response[4]; + unsigned int flags; + unsigned int blksz; + unsigned int blocks; + unsigned int postsleep_min_us; + unsigned int postsleep_max_us; + unsigned int data_timeout_ns; + unsigned int cmd_timeout_ms; + __u32 __pad; + __u64 data_ptr; +}; + +struct compat_mmc_ioc_multi_cmd { + __u64 num_of_cmds; + struct compat_mmc_ioc_cmd cmds[]; +}; + /* * There is one mmc_blk_data per slot. */ @@ -401,6 +428,47 @@ struct mmc_blk_ioc_data { struct mmc_rpmb_data *rpmb; };
+/* + * Copy the data from a compat_mmc_ioc_cmd user pointer, src, + * to kernel space, storing it in native_cmd. Returns 0 for + * a successful copy. + */ +static int get_mmc_ioc_cmd_from_compat64(struct mmc_ioc_cmd *native_cmd, + void * __user src) +{ + struct compat_mmc_ioc_cmd compat_cmd; + + if (copy_from_user(&compat_cmd, src, sizeof(struct compat_mmc_ioc_cmd))) + return -EFAULT; + + native_cmd->arg = compat_cmd.arg; + native_cmd->is_acmd = compat_cmd.is_acmd; + native_cmd->opcode = compat_cmd.opcode; + native_cmd->arg = compat_cmd.arg; + memcpy(native_cmd->response, compat_cmd.response, sizeof(compat_cmd.response)); + native_cmd->flags = compat_cmd.flags; + native_cmd->blksz = compat_cmd.blksz; + native_cmd->blocks = compat_cmd.blocks; + native_cmd->postsleep_min_us = compat_cmd.postsleep_min_us; + native_cmd->postsleep_max_us = compat_cmd.postsleep_max_us; + native_cmd->data_timeout_ns = compat_cmd.data_timeout_ns; + native_cmd->cmd_timeout_ms = compat_cmd.cmd_timeout_ms; + native_cmd->__pad = compat_cmd.__pad; + native_cmd->data_ptr = compat_cmd.data_ptr; + + return 0; +} + +static int copy_mmc_ioc_cmd_from_user(struct mmc_ioc_cmd *to, void * __user src) +{ + if (in_compat64()) + return get_mmc_ioc_cmd_from_compat64(to, src); + + if (copy_from_user(to, src, sizeof(*to))) + return -EFAULT; + return 0; +} + static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( struct mmc_ioc_cmd __user *user) { @@ -413,7 +481,7 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( goto out; }
- if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) { + if (copy_mmc_ioc_cmd_from_user(&idata->ic, user)) { err = -EFAULT; goto idata_err; } @@ -449,8 +517,11 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, { struct mmc_ioc_cmd *ic = &idata->ic;
- if (copy_to_user(&(ic_ptr->response), ic->response, - sizeof(ic->response))) + __u32 __user *response_uptr = in_compat64() ? + &((struct compat_mmc_ioc_cmd __user *)ic_ptr)->response[0] : + &ic_ptr->response[0]; + + if (copy_to_user(response_uptr, ic->response, sizeof(ic->response))) return -EFAULT;
if (!idata->ic.write_flag) { @@ -666,12 +737,20 @@ static int mmc_blk_ioctl_cmd(struct mmc_blk_data *md, return ioc_err ? ioc_err : err; }
+static inline struct mmc_ioc_cmd __user *get_ith_mmc_ioc_cmd_uptr( + struct mmc_ioc_multi_cmd __user *user, + unsigned int i) +{ + if (in_compat64()) + return (struct mmc_ioc_cmd __user *)&((struct compat_mmc_ioc_multi_cmd __user *)user)->cmds[i]; + return &(user->cmds[i]); +} + static int mmc_blk_ioctl_multi_cmd(struct mmc_blk_data *md, struct mmc_ioc_multi_cmd __user *user, struct mmc_rpmb_data *rpmb) { struct mmc_blk_ioc_data **idata = NULL; - struct mmc_ioc_cmd __user *cmds = user->cmds; struct mmc_card *card; struct mmc_queue *mq; int err = 0, ioc_err = 0; @@ -679,6 +758,11 @@ static int mmc_blk_ioctl_multi_cmd(struct mmc_blk_data *md, unsigned int i, n; struct request *req;
+ /* + * Both native and compat64 versions of mmc_ioc_multi_cmd + * have num_of_cmds as the first field, so the offset does + * not need to be recalculated for compat64. + */ if (copy_from_user(&num_of_cmds, &user->num_of_cmds, sizeof(num_of_cmds))) return -EFAULT; @@ -695,7 +779,7 @@ static int mmc_blk_ioctl_multi_cmd(struct mmc_blk_data *md, return -ENOMEM;
for (i = 0; i < n; i++) { - idata[i] = mmc_blk_ioctl_copy_from_user(&cmds[i]); + idata[i] = mmc_blk_ioctl_copy_from_user(get_ith_mmc_ioc_cmd_uptr(user, i)); if (IS_ERR(idata[i])) { err = PTR_ERR(idata[i]); n = i; @@ -732,7 +816,7 @@ static int mmc_blk_ioctl_multi_cmd(struct mmc_blk_data *md,
/* copy to user if data and response */ for (i = 0; i < n && !err; i++) - err = mmc_blk_ioctl_copy_to_user(&cmds[i], idata[i]); + err = mmc_blk_ioctl_copy_to_user(get_ith_mmc_ioc_cmd_uptr(user, i), idata[i]);
blk_mq_free_request(req);
The mmc_ioc_cmd and mmc_ioc_multi_cmd structs are used to hold information and data about an MMC ioctl.
The PCuABI uses 129-bit capabilities as user pointers, which means that the __u64 type must be replaced with the __kernel_uintptr_t type, which is large enough to hold capabilities, yet will remain 64-bit on other architectures. Additional modifications have been made the copy routines.
The command value for an ioctl is inferred from the size of the struct that is provided to it; this patch increases the size of struct mmc_ioc_cmd with the use of __kernel_uintptr_t, which in turn modifies the calculated value for the ioctl. In order to prevent this value changing, it is necessary to hard-code the definition of the MMC_IOC_CMD ioctl value, as compat structs are not defined in this uAPI header.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com --- drivers/mmc/core/block.c | 8 ++++---- include/uapi/linux/mmc/ioctl.h | 19 ++++++++++++++++--- 2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 3333c4917028..547941096c0e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -454,7 +454,7 @@ static int get_mmc_ioc_cmd_from_compat64(struct mmc_ioc_cmd *native_cmd, native_cmd->data_timeout_ns = compat_cmd.data_timeout_ns; native_cmd->cmd_timeout_ms = compat_cmd.cmd_timeout_ms; native_cmd->__pad = compat_cmd.__pad; - native_cmd->data_ptr = compat_cmd.data_ptr; + native_cmd->data_ptr = (__kernel_uintptr_t)compat_ptr(compat_cmd.data_ptr);
return 0; } @@ -464,7 +464,7 @@ static int copy_mmc_ioc_cmd_from_user(struct mmc_ioc_cmd *to, void * __user src) if (in_compat64()) return get_mmc_ioc_cmd_from_compat64(to, src);
- if (copy_from_user(to, src, sizeof(*to))) + if (copy_from_user_with_ptr(to, src, sizeof(*to))) return -EFAULT; return 0; } @@ -497,7 +497,7 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( return idata; }
- idata->buf = memdup_user(uaddr_to_user_ptr(idata->ic.data_ptr), + idata->buf = memdup_user((void __user *)idata->ic.data_ptr, idata->buf_bytes); if (IS_ERR(idata->buf)) { err = PTR_ERR(idata->buf); @@ -525,7 +525,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, return -EFAULT;
if (!idata->ic.write_flag) { - if (copy_to_user(uaddr_to_user_ptr(ic->data_ptr), + if (copy_to_user((void __user *)ic->data_ptr, idata->buf, idata->buf_bytes)) return -EFAULT; } diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h index e7401ade6822..ce411b555d03 100644 --- a/include/uapi/linux/mmc/ioctl.h +++ b/include/uapi/linux/mmc/ioctl.h @@ -2,6 +2,9 @@ #ifndef LINUX_MMC_IOCTL_H #define LINUX_MMC_IOCTL_H
+#ifndef __KERNEL__ +#include <stdint.h> +#endif #include <linux/types.h> #include <linux/major.h>
@@ -46,9 +49,9 @@ struct mmc_ioc_cmd { __u32 __pad;
/* DAT buffer */ - __u64 data_ptr; + __kernel_uintptr_t data_ptr; }; -#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr +#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (uintptr_t) ptr
/** * struct mmc_ioc_multi_cmd - multi command information @@ -61,7 +64,17 @@ struct mmc_ioc_multi_cmd { struct mmc_ioc_cmd cmds[]; };
-#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd) +/* + * The size of struct mmc_ioc_cmd has changed 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) + */ +#define MMC_IOC_CMD _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 72) /* * 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
On 15/11/2023 19:15, Akram Ahmad wrote:
The mmc_ioc_cmd and mmc_ioc_multi_cmd structs are used to hold information and data about an MMC ioctl.
The PCuABI uses 129-bit capabilities as user pointers, which means that the __u64 type must be replaced with the __kernel_uintptr_t type, which is large enough to hold capabilities, yet will remain 64-bit on other architectures. Additional modifications have been made the copy routines.
The command value for an ioctl is inferred from the size of the struct that is provided to it; this patch increases the size of struct mmc_ioc_cmd with the use of __kernel_uintptr_t, which in turn modifies the calculated value for the ioctl. In order to prevent this value changing, it is necessary to hard-code the definition of the MMC_IOC_CMD ioctl value, as compat structs are not defined in this uAPI header.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com
drivers/mmc/core/block.c | 8 ++++---- include/uapi/linux/mmc/ioctl.h | 19 ++++++++++++++++--- 2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 3333c4917028..547941096c0e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -454,7 +454,7 @@ static int get_mmc_ioc_cmd_from_compat64(struct mmc_ioc_cmd *native_cmd, native_cmd->data_timeout_ns = compat_cmd.data_timeout_ns; native_cmd->cmd_timeout_ms = compat_cmd.cmd_timeout_ms; native_cmd->__pad = compat_cmd.__pad;
- native_cmd->data_ptr = compat_cmd.data_ptr;
- native_cmd->data_ptr = (__kernel_uintptr_t)compat_ptr(compat_cmd.data_ptr);
return 0; } @@ -464,7 +464,7 @@ static int copy_mmc_ioc_cmd_from_user(struct mmc_ioc_cmd *to, void * __user src) if (in_compat64()) return get_mmc_ioc_cmd_from_compat64(to, src);
- if (copy_from_user(to, src, sizeof(*to)))
- if (copy_from_user_with_ptr(to, src, sizeof(*to))) return -EFAULT; return 0;
} @@ -497,7 +497,7 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( return idata; }
- idata->buf = memdup_user(uaddr_to_user_ptr(idata->ic.data_ptr),
- idata->buf = memdup_user((void __user *)idata->ic.data_ptr, idata->buf_bytes); if (IS_ERR(idata->buf)) { err = PTR_ERR(idata->buf);
@@ -525,7 +525,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, return -EFAULT; if (!idata->ic.write_flag) {
if (copy_to_user(uaddr_to_user_ptr(ic->data_ptr),
if (copy_to_user((void __user *)ic->data_ptr, idata->buf, idata->buf_bytes)) return -EFAULT;diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h index e7401ade6822..ce411b555d03 100644 --- a/include/uapi/linux/mmc/ioctl.h +++ b/include/uapi/linux/mmc/ioctl.h @@ -2,6 +2,9 @@ #ifndef LINUX_MMC_IOCTL_H #define LINUX_MMC_IOCTL_H +#ifndef __KERNEL__ +#include <stdint.h> +#endif #include <linux/types.h> #include <linux/major.h> @@ -46,9 +49,9 @@ struct mmc_ioc_cmd { __u32 __pad; /* DAT buffer */
- __u64 data_ptr;
- __kernel_uintptr_t data_ptr;
}; -#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr +#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (uintptr_t) ptr /**
- struct mmc_ioc_multi_cmd - multi command information
@@ -61,7 +64,17 @@ struct mmc_ioc_multi_cmd { struct mmc_ioc_cmd cmds[]; }; -#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd) +/*
- The size of struct mmc_ioc_cmd has changed 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)
- */
+#define MMC_IOC_CMD _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 72) /*
- MMC_IOC_MULTI_CMD: Used to send an array of MMC commands described by
My comment in v6 on MMC_IOC_MULTI_CMD was wrong, sorry for that. I had not realised that the increased alignment of struct mmc_ioc_cmd would itself increase the alignment of struct mmc_ioc_multi_cmd, and since its size was just 8, it gets bumped to 16.
I have amended the definition of MMC_IOC_MULTI_CMD like in v6 and updated the comment and commit message accordingly. With that, ioctl()s issued by mmc-utils seem to be at least understood. The series is now in next, thank you for your persistence!
Kevin
On 20/11/2023 09:20, Kevin Brodsky wrote:
On 15/11/2023 19:15, Akram Ahmad wrote:
The mmc_ioc_cmd and mmc_ioc_multi_cmd structs are used to hold information and data about an MMC ioctl.
The PCuABI uses 129-bit capabilities as user pointers, which means that the __u64 type must be replaced with the __kernel_uintptr_t type, which is large enough to hold capabilities, yet will remain 64-bit on other architectures. Additional modifications have been made the copy routines.
The command value for an ioctl is inferred from the size of the struct that is provided to it; this patch increases the size of struct mmc_ioc_cmd with the use of __kernel_uintptr_t, which in turn modifies the calculated value for the ioctl. In order to prevent this value changing, it is necessary to hard-code the definition of the MMC_IOC_CMD ioctl value, as compat structs are not defined in this uAPI header.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com
drivers/mmc/core/block.c | 8 ++++---- include/uapi/linux/mmc/ioctl.h | 19 ++++++++++++++++--- 2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 3333c4917028..547941096c0e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -454,7 +454,7 @@ static int get_mmc_ioc_cmd_from_compat64(struct mmc_ioc_cmd *native_cmd, native_cmd->data_timeout_ns = compat_cmd.data_timeout_ns; native_cmd->cmd_timeout_ms = compat_cmd.cmd_timeout_ms; native_cmd->__pad = compat_cmd.__pad;
- native_cmd->data_ptr = compat_cmd.data_ptr;
- native_cmd->data_ptr = (__kernel_uintptr_t)compat_ptr(compat_cmd.data_ptr);
return 0; } @@ -464,7 +464,7 @@ static int copy_mmc_ioc_cmd_from_user(struct mmc_ioc_cmd *to, void * __user src) if (in_compat64()) return get_mmc_ioc_cmd_from_compat64(to, src);
- if (copy_from_user(to, src, sizeof(*to)))
- if (copy_from_user_with_ptr(to, src, sizeof(*to))) return -EFAULT; return 0; }
@@ -497,7 +497,7 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( return idata; }
- idata->buf = memdup_user(uaddr_to_user_ptr(idata->ic.data_ptr),
- idata->buf = memdup_user((void __user *)idata->ic.data_ptr, idata->buf_bytes); if (IS_ERR(idata->buf)) { err = PTR_ERR(idata->buf);
@@ -525,7 +525,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, return -EFAULT; if (!idata->ic.write_flag) {
if (copy_to_user(uaddr_to_user_ptr(ic->data_ptr),
if (copy_to_user((void __user *)ic->data_ptr, idata->buf, idata->buf_bytes)) return -EFAULT;diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h index e7401ade6822..ce411b555d03 100644 --- a/include/uapi/linux/mmc/ioctl.h +++ b/include/uapi/linux/mmc/ioctl.h @@ -2,6 +2,9 @@ #ifndef LINUX_MMC_IOCTL_H #define LINUX_MMC_IOCTL_H +#ifndef __KERNEL__ +#include <stdint.h> +#endif #include <linux/types.h> #include <linux/major.h> @@ -46,9 +49,9 @@ struct mmc_ioc_cmd { __u32 __pad; /* DAT buffer */
- __u64 data_ptr;
- __kernel_uintptr_t data_ptr; };
-#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr +#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (uintptr_t) ptr /**
- struct mmc_ioc_multi_cmd - multi command information
@@ -61,7 +64,17 @@ struct mmc_ioc_multi_cmd { struct mmc_ioc_cmd cmds[]; }; -#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd) +/*
- The size of struct mmc_ioc_cmd has changed 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)
- */
+#define MMC_IOC_CMD _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 72) /*
- MMC_IOC_MULTI_CMD: Used to send an array of MMC commands described by
My comment in v6 on MMC_IOC_MULTI_CMD was wrong, sorry for that. I had not realised that the increased alignment of struct mmc_ioc_cmd would itself increase the alignment of struct mmc_ioc_multi_cmd, and since its size was just 8, it gets bumped to 16.
I have amended the definition of MMC_IOC_MULTI_CMD like in v6 and updated the comment and commit message accordingly. With that, ioctl()s issued by mmc-utils seem to be at least understood. The series is now in next, thank you for your persistence!
Kevin
No worries, thank you for all of the help and guidance you've provided for me on my first patch!
Akram
linux-morello@op-lists.linaro.org
-
Akram Ahmad -
Kevin Brodsky