[linux-morello] Re: [PATCH V2] mmc: Implement compat handling for struct mmc_ioc_{multi_}cmd

On 17/10/2023 18:29, Akram Ahmad wrote:

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.

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

This is useful but changelogs are not meant to become part of the Git history, in other words they shouldn't be in the commit message. When sending a single patch like this, they can be added (along with any other comment) after the --- below the commit message.

Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com

drivers/mmc/core/block.c | 95 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 90 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 2f8884e3d4c2..311944c82313 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,48 @@ struct mmc_blk_ioc_data { struct mmc_rpmb_data *rpmb; }; +/*

• • Copy the data from a `compat_mmc_ioc_cmd` user pointer, `src`,

Same comment as in v1 regarding backticks.

• • 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,

My comment in v1 regarding argument formatting applied to all functions: the first argument should be on the same line, and any overflowing argument aligned on the opening parenthesis, if possible.

• 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 +482,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 +518,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) :
  

• 	*(&ic_ptr->response);
  

I don't understand the * at the beginning of this line and the previous one.

• if (copy_to_user(response_uptr, ic->response, sizeof(ic->response))) return -EFAULT;

if (!idata->ic.write_flag) { @@ -666,6 +738,15 @@ 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_cmd __user *cmds_array,
• unsigned int i)

+{

• if (in_compat64())

• return (struct mmc_ioc_cmd __user *)&((struct compat_mmc_ioc_cmd __user *)cmds_array)[i];
  

• return &(cmds_array)[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) @@ -679,6 +760,10 @@ 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 based on the differing
• // layouts.

C++-style comments are discouraged in the kernel, better use C-style (/* ... */).

Also "differing layouts" might be confusing, as the point is precisely that the layout is the same for this field.

if (copy_from_user(&num_of_cmds, &user->num_of_cmds, sizeof(num_of_cmds))) return -EFAULT; @@ -695,7 +780,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(cmds, i));
  

We can't do this, cmds is only valid in native (the offset of the cmds field in struct mmc_ioc_multi_cmd is different in PCuABI because the alignment of struct mmc_ioc_cmd is 16 instead of 8). My point in v1 was that get_ith_mmc_ioc_cmd_uptr() could take the user pointer, and then use the right layout to access cmds.

Kevin

if (IS_ERR(idata[i])) {
	err = PTR_ERR(idata[i]);
	n = i;

@@ -732,7 +817,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(cmds, i), idata[i]);
  

blk_mq_free_request(req);