| Message ID | 20220324011126.1144504-2-alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Do not allow set-partition immediate mode | expand |
On Wed, 23 Mar 2022 18:11:18 -0700 alison.schofield@intel.com wrote: > From: Alison Schofield <alison.schofield@intel.com> > > Sanitizing and constructing a cxl_mem_command from a userspace > command is part of the validation process prior to submitting > the command to a CXL device. Move this work to helper functions: > cxl_to_mem_cmd(), cxl_to_mem_cmd_raw(). > > This declutters cxl_validate_cmd_from_user() in preparation for > adding new validation steps. > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> A few trivial comments inline. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/mbox.c | 158 +++++++++++++++++++++------------------- > 1 file changed, 85 insertions(+), 73 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index be61a0d8016b..6612d73c37a8 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -207,6 +207,84 @@ static bool cxl_mem_raw_command_allowed(u16 opcode) > return true; > } > > +static int cxl_to_mem_cmd_raw(struct cxl_dev_state *cxlds, > + const struct cxl_send_command *send_cmd, > + struct cxl_mem_command *mem_cmd) > +{ > + if (send_cmd->raw.rsvd) > + return -EINVAL; trivial: Blank line here would be good. > + /* > + * Unlike supported commands, the output size of RAW commands > + * gets passed along without further checking, so it must be > + * validated here. > + */ > + if (send_cmd->out.size > cxlds->payload_size) > + return -EINVAL; > + > + if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode)) > + return -EPERM; > + > + *mem_cmd = (struct cxl_mem_command) { > + .info = { > + .id = CXL_MEM_COMMAND_ID_RAW, > + .size_in = send_cmd->in.size, > + .size_out = send_cmd->out.size, > + }, > + .opcode = send_cmd->raw.opcode > + }; > + > + return 0; > +} > + > +static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds, > + const struct cxl_send_command *send_cmd, > + struct cxl_mem_command *mem_cmd) > +{ > + const struct cxl_command_info *info; > + struct cxl_mem_command *c; > + > + if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > + return -EINVAL; > + > + if (send_cmd->rsvd) > + return -EINVAL; > + > + if (send_cmd->in.rsvd || send_cmd->out.rsvd) > + return -EINVAL; > + > + /* Convert user's command into the internal representation */ Not clear which chunk of code this applies to. Seems like we are just getting some addresses here (obviously that applies to original code as well) Perhaps move down to where you fill in mem_cmd? > + c = &cxl_mem_commands[send_cmd->id]; > + info = &c->info; I don't mind that much either way, but you could do these at declaration of the local variables above, before doing the sanity checks. > + > + /* Check that the command is enabled for hardware */ > + if (!test_bit(info->id, cxlds->enabled_cmds)) > + return -ENOTTY; > + > + /* Check that the command is not claimed for exclusive kernel use */ > + if (test_bit(info->id, cxlds->exclusive_cmds)) > + return -EBUSY; > + > + /* Check the input buffer is the expected size */ > + if (info->size_in >= 0 && info->size_in != send_cmd->in.size) > + return -ENOMEM; > + > + /* Check the output buffer is at least large enough */ > + if (info->size_out >= 0 && send_cmd->out.size < info->size_out) > + return -ENOMEM; > + > + *mem_cmd = (struct cxl_mem_command) { > + .info = { > + .id = info->id, > + .flags = info->flags, > + .size_in = send_cmd->in.size, > + .size_out = send_cmd->out.size, > + }, > + .opcode = c->opcode > + }; > + > + return 0; > +} > + > /** > * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND. > * @cxlds: The device data for the operation > @@ -230,8 +308,7 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds, > const struct cxl_send_command *send_cmd, > struct cxl_mem_command *out_cmd) > { > - const struct cxl_command_info *info; > - struct cxl_mem_command *c; > + int rc; > > if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX) > return -ENOTTY; > @@ -244,78 +321,13 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds, > if (send_cmd->in.size > cxlds->payload_size) > return -EINVAL; > > - /* > - * Checks are bypassed for raw commands but a WARN/taint will occur > - * later in the callchain > - */ > - if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW) { > - const struct cxl_mem_command temp = { > - .info = { > - .id = CXL_MEM_COMMAND_ID_RAW, > - .flags = 0, > - .size_in = send_cmd->in.size, > - .size_out = send_cmd->out.size, > - }, > - .opcode = send_cmd->raw.opcode > - }; > + /* Sanitize and construct a cxl_mem_command */ > + if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW) > + rc = cxl_to_mem_cmd_raw(cxlds, send_cmd, out_cmd); > + else > + rc = cxl_to_mem_cmd(cxlds, send_cmd, out_cmd); > > - if (send_cmd->raw.rsvd) > - return -EINVAL; > - > - /* > - * Unlike supported commands, the output size of RAW commands > - * gets passed along without further checking, so it must be > - * validated here. > - */ > - if (send_cmd->out.size > cxlds->payload_size) > - return -EINVAL; > - > - if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode)) > - return -EPERM; > - > - memcpy(out_cmd, &temp, sizeof(temp)); > - > - return 0; > - } > - > - if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > - return -EINVAL; > - > - if (send_cmd->rsvd) > - return -EINVAL; > - > - if (send_cmd->in.rsvd || send_cmd->out.rsvd) > - return -EINVAL; > - > - /* Convert user's command into the internal representation */ > - c = &cxl_mem_commands[send_cmd->id]; > - info = &c->info; > - > - /* Check that the command is enabled for hardware */ > - if (!test_bit(info->id, cxlds->enabled_cmds)) > - return -ENOTTY; > - > - /* Check that the command is not claimed for exclusive kernel use */ > - if (test_bit(info->id, cxlds->exclusive_cmds)) > - return -EBUSY; > - > - /* Check the input buffer is the expected size */ > - if (info->size_in >= 0 && info->size_in != send_cmd->in.size) > - return -ENOMEM; > - > - /* Check the output buffer is at least large enough */ > - if (info->size_out >= 0 && send_cmd->out.size < info->size_out) > - return -ENOMEM; > - > - memcpy(out_cmd, c, sizeof(*c)); > - out_cmd->info.size_in = send_cmd->in.size; > - /* > - * XXX: out_cmd->info.size_out will be controlled by the driver, and the > - * specified number of bytes @send_cmd->out.size will be copied back out > - * to userspace. > - */ > - > - return 0; > + return rc; I haven't read on yet so I'll assume there is more coming in this function as otherwise you could just return directly in the two if / else paths. Thanks, Jonathan > } > > int cxl_query_cmd(struct cxl_memdev *cxlmd,
On Fri, Mar 25, 2022 at 10:27:24AM +0000, Jonathan Cameron wrote: > On Wed, 23 Mar 2022 18:11:18 -0700 > alison.schofield@intel.com wrote: > > > From: Alison Schofield <alison.schofield@intel.com> > > > > Sanitizing and constructing a cxl_mem_command from a userspace > > command is part of the validation process prior to submitting > > the command to a CXL device. Move this work to helper functions: > > cxl_to_mem_cmd(), cxl_to_mem_cmd_raw(). > > > > This declutters cxl_validate_cmd_from_user() in preparation for > > adding new validation steps. > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > A few trivial comments inline. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Thanks for the review Jonathan - > > --- snip > > > > +static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds, > > + const struct cxl_send_command *send_cmd, > > + struct cxl_mem_command *mem_cmd) > > +{ > > + const struct cxl_command_info *info; > > + struct cxl_mem_command *c; > > + > > + if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) > > + return -EINVAL; > > + > > + if (send_cmd->rsvd) > > + return -EINVAL; > > + > > + if (send_cmd->in.rsvd || send_cmd->out.rsvd) > > + return -EINVAL; > > + > > + /* Convert user's command into the internal representation */ > > Not clear which chunk of code this applies to. Seems like we > are just getting some addresses here (obviously that applies to original > code as well) Perhaps move down to where you fill in mem_cmd? The comment above applies to the line that immediately follow. ie. senc_cmd->id indexes into the command array. > > > + c = &cxl_mem_commands[send_cmd->id]; > > + info = &c->info; > > I don't mind that much either way, but you could do these at > declaration of the local variables above, before doing the sanity checks. > I err'd on moving things as much 'as is' as possible to keep the patches easier to review. I like your suggestion and will do it. (Please like it when you see it in the next version ;)) > > + snip > > - > > - return 0; > > + return rc; > I haven't read on yet so I'll assume there is more coming in this function as otherwise > you could just return directly in the two if / else paths. Yeah, you are seeing some foreshadowing, and you've noted similar in other patches. I'll clean that up. Thanks, Alison > > Thanks, > > Jonathan > > > } > > > > int cxl_query_cmd(struct cxl_memdev *cxlmd, >
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index be61a0d8016b..6612d73c37a8 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -207,6 +207,84 @@ static bool cxl_mem_raw_command_allowed(u16 opcode) return true; } +static int cxl_to_mem_cmd_raw(struct cxl_dev_state *cxlds, + const struct cxl_send_command *send_cmd, + struct cxl_mem_command *mem_cmd) +{ + if (send_cmd->raw.rsvd) + return -EINVAL; + /* + * Unlike supported commands, the output size of RAW commands + * gets passed along without further checking, so it must be + * validated here. + */ + if (send_cmd->out.size > cxlds->payload_size) + return -EINVAL; + + if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode)) + return -EPERM; + + *mem_cmd = (struct cxl_mem_command) { + .info = { + .id = CXL_MEM_COMMAND_ID_RAW, + .size_in = send_cmd->in.size, + .size_out = send_cmd->out.size, + }, + .opcode = send_cmd->raw.opcode + }; + + return 0; +} + +static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds, + const struct cxl_send_command *send_cmd, + struct cxl_mem_command *mem_cmd) +{ + const struct cxl_command_info *info; + struct cxl_mem_command *c; + + if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) + return -EINVAL; + + if (send_cmd->rsvd) + return -EINVAL; + + if (send_cmd->in.rsvd || send_cmd->out.rsvd) + return -EINVAL; + + /* Convert user's command into the internal representation */ + c = &cxl_mem_commands[send_cmd->id]; + info = &c->info; + + /* Check that the command is enabled for hardware */ + if (!test_bit(info->id, cxlds->enabled_cmds)) + return -ENOTTY; + + /* Check that the command is not claimed for exclusive kernel use */ + if (test_bit(info->id, cxlds->exclusive_cmds)) + return -EBUSY; + + /* Check the input buffer is the expected size */ + if (info->size_in >= 0 && info->size_in != send_cmd->in.size) + return -ENOMEM; + + /* Check the output buffer is at least large enough */ + if (info->size_out >= 0 && send_cmd->out.size < info->size_out) + return -ENOMEM; + + *mem_cmd = (struct cxl_mem_command) { + .info = { + .id = info->id, + .flags = info->flags, + .size_in = send_cmd->in.size, + .size_out = send_cmd->out.size, + }, + .opcode = c->opcode + }; + + return 0; +} + /** * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND. * @cxlds: The device data for the operation @@ -230,8 +308,7 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds, const struct cxl_send_command *send_cmd, struct cxl_mem_command *out_cmd) { - const struct cxl_command_info *info; - struct cxl_mem_command *c; + int rc; if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX) return -ENOTTY; @@ -244,78 +321,13 @@ static int cxl_validate_cmd_from_user(struct cxl_dev_state *cxlds, if (send_cmd->in.size > cxlds->payload_size) return -EINVAL; - /* - * Checks are bypassed for raw commands but a WARN/taint will occur - * later in the callchain - */ - if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW) { - const struct cxl_mem_command temp = { - .info = { - .id = CXL_MEM_COMMAND_ID_RAW, - .flags = 0, - .size_in = send_cmd->in.size, - .size_out = send_cmd->out.size, - }, - .opcode = send_cmd->raw.opcode - }; + /* Sanitize and construct a cxl_mem_command */ + if (send_cmd->id == CXL_MEM_COMMAND_ID_RAW) + rc = cxl_to_mem_cmd_raw(cxlds, send_cmd, out_cmd); + else + rc = cxl_to_mem_cmd(cxlds, send_cmd, out_cmd); - if (send_cmd->raw.rsvd) - return -EINVAL; - - /* - * Unlike supported commands, the output size of RAW commands - * gets passed along without further checking, so it must be - * validated here. - */ - if (send_cmd->out.size > cxlds->payload_size) - return -EINVAL; - - if (!cxl_mem_raw_command_allowed(send_cmd->raw.opcode)) - return -EPERM; - - memcpy(out_cmd, &temp, sizeof(temp)); - - return 0; - } - - if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK) - return -EINVAL; - - if (send_cmd->rsvd) - return -EINVAL; - - if (send_cmd->in.rsvd || send_cmd->out.rsvd) - return -EINVAL; - - /* Convert user's command into the internal representation */ - c = &cxl_mem_commands[send_cmd->id]; - info = &c->info; - - /* Check that the command is enabled for hardware */ - if (!test_bit(info->id, cxlds->enabled_cmds)) - return -ENOTTY; - - /* Check that the command is not claimed for exclusive kernel use */ - if (test_bit(info->id, cxlds->exclusive_cmds)) - return -EBUSY; - - /* Check the input buffer is the expected size */ - if (info->size_in >= 0 && info->size_in != send_cmd->in.size) - return -ENOMEM; - - /* Check the output buffer is at least large enough */ - if (info->size_out >= 0 && send_cmd->out.size < info->size_out) - return -ENOMEM; - - memcpy(out_cmd, c, sizeof(*c)); - out_cmd->info.size_in = send_cmd->in.size; - /* - * XXX: out_cmd->info.size_out will be controlled by the driver, and the - * specified number of bytes @send_cmd->out.size will be copied back out - * to userspace. - */ - - return 0; + return rc; } int cxl_query_cmd(struct cxl_memdev *cxlmd,