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.
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_v3
Akram Ahmad (1): mmc: Implement compat handling for struct mmc_ioc_{multi_}cmd mmc: Support capabilities in MMC_IOC_{MULTI_}CMD ioctls
drivers/mmc/core/block.c | 108 ++++++++++++++++++++++++++++++--- include/uapi/linux/mmc/ioctl.h | 4 +- 2 files changed, 102 insertions(+), 10 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..8ded96ea629b 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);
On 24/10/2023 14:35, Akram Ahmad wrote:
[...] +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];
Can get rid of the parentheses around user, just to reduce the bracket-fest a bit...
Kevin
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 must be made to the copy routines, and some explicit checks on the permissions of the capabilities have also been added.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com --- drivers/mmc/core/block.c | 16 ++++++++++++---- include/uapi/linux/mmc/ioctl.h | 4 ++-- 2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 8ded96ea629b..d027c70dc79b 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; } @@ -481,6 +481,11 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( goto out; }
+ if (!check_user_ptr_read(&user->data_ptr, sizeof(user->data_ptr))) { + err = -EFAULT; + goto idata_err; + } + if (copy_mmc_ioc_cmd_from_user(&idata->ic, user)) { err = -EFAULT; goto idata_err; @@ -497,7 +502,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); @@ -524,8 +529,11 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, if (copy_to_user(response_uptr, ic->response, sizeof(ic->response))) return -EFAULT;
+ if (!check_user_ptr_write((void __user *)ic->data_ptr, idata->buf_bytes)) + return -EFAULT; + if (!idata->ic.write_flag) { - if (copy_to_user(uaddr_to_user_ptr(ic->data_ptr), + if (copy_to_user_with_ptr((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..6caf1239d993 100644 --- a/include/uapi/linux/mmc/ioctl.h +++ b/include/uapi/linux/mmc/ioctl.h @@ -46,7 +46,7 @@ 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
@@ -57,7 +57,7 @@ struct mmc_ioc_cmd { * @cmds: Array of commands with length equal to 'num_of_cmds' */ struct mmc_ioc_multi_cmd { - __u64 num_of_cmds; + __kernel_uintptr_t num_of_cmds; struct mmc_ioc_cmd cmds[]; };
On 24/10/2023 14:35, 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 must be made to the copy routines, and some explicit checks on the permissions of the capabilities have also been added.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com
drivers/mmc/core/block.c | 16 ++++++++++++---- include/uapi/linux/mmc/ioctl.h | 4 ++-- 2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 8ded96ea629b..d027c70dc79b 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;
} @@ -481,6 +481,11 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( goto out; }
- if (!check_user_ptr_read(&user->data_ptr, sizeof(user->data_ptr))) {
Explicit checking is only required in those rare cases where user memory is accessed via a kernel mapping. Here we are passing data_ptr to memdup_user(), which itself copies the user data using copy_from_user(), i.e. normal uaccess that already does the checking "for free".
This line should also give you a warning as &user->data_ptr is not a capability, it's a regular pointer (to a capability).
err = -EFAULT;goto idata_err;- }
- if (copy_mmc_ioc_cmd_from_user(&idata->ic, user)) { err = -EFAULT; goto idata_err;
@@ -497,7 +502,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),
No need for parentheses around idata->ic.data_ptr.
idata->buf_bytes);if (IS_ERR(idata->buf)) { err = PTR_ERR(idata->buf); @@ -524,8 +529,11 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, if (copy_to_user(response_uptr, ic->response, sizeof(ic->response))) return -EFAULT;
- if (!check_user_ptr_write((void __user *)ic->data_ptr, idata->buf_bytes))
Like above, we are using regular uaccess so no need for explicit checking.
return -EFAULT;- if (!idata->ic.write_flag) {
if (copy_to_user(uaddr_to_user_ptr(ic->data_ptr),
if (copy_to_user_with_ptr((void __user *)(ic->data_ptr), idata->buf, idata->buf_bytes))
Does idata->buf actually contain pointers? We should only use copy_*_user_with_ptr() when the data to be copied contains pointers.
return -EFAULT;} diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h index e7401ade6822..6caf1239d993 100644 --- a/include/uapi/linux/mmc/ioctl.h +++ b/include/uapi/linux/mmc/ioctl.h @@ -46,7 +46,7 @@ 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 @@ -57,7 +57,7 @@ struct mmc_ioc_cmd {
- @cmds: Array of commands with length equal to 'num_of_cmds'
*/ struct mmc_ioc_multi_cmd {
- __u64 num_of_cmds;
- __kernel_uintptr_t num_of_cmds;
That's not a pointer, no need to change its representation in PCuABI. __u64s in uapi headers often represent pointers but not quite always!
Kevin
struct mmc_ioc_cmd cmds[]; };
linux-morello@op-lists.linaro.org
-
Akram Ahmad -
Kevin Brodsky