| Message ID | 84c0ddda79331bbbc37a6237e980b53735c29c77.1645817416.git.alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Do not allow set-partition immediate mode | expand |
On Fri, Feb 25, 2022 at 12:27 PM <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> > --- > drivers/cxl/core/mbox.c | 143 ++++++++++++++++++++++------------------ > 1 file changed, 79 insertions(+), 64 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index be61a0d8016b..06fbe6d079ba 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -207,6 +207,75 @@ 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; > + > + dev_WARN_ONCE(cxlds->dev, true, "raw command path used\n"); > + > + mem_cmd->info.id = CXL_MEM_COMMAND_ID_RAW; > + mem_cmd->info.flags = 0; > + mem_cmd->info.size_in = send_cmd->in.size; > + mem_cmd->info.size_out = send_cmd->out.size; > + mem_cmd->opcode = send_cmd->raw.opcode; One subtle change from the previous version to this one is that it no longer gets automatic zero initialization by the compiler. This leaves a subtle timebomb if 'struct cxl_mem_command' is ever updated. So I would write this as: *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 }; ...where you can skip setting flags to 0 since that is implied by the syntax and there is no need to worry about if all the unwritten fields are zeroed. > + > + 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; > + > + memcpy(mem_cmd, c, sizeof(*c)); > + mem_cmd->info.size_in = send_cmd->in.size; Perhaps for symmetry this can become: *mem_cmd = (struct cxl_mem_command) { .info = { .id = info->id, .flags = info->flags, .size_in = send_cmd->in.size, .size_out = info->size_out, }, .opcode = c->opcode }; ...if I got that right... > + return 0; > +} > + > /** > * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND. > * @cxlds: The device data for the operation > @@ -230,8 +299,8 @@ 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; > + struct cxl_mem_command mem_cmd; > + int rc; > > if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX) > return -ENOTTY; > @@ -244,70 +313,16 @@ 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, &mem_cmd); > + else > + rc = cxl_to_mem_cmd(cxlds, send_cmd, &mem_cmd); > > - if (send_cmd->raw.rsvd) > - return -EINVAL; > + if (rc) > + return rc; > > - /* > - * 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)); > + memcpy(out_cmd, &mem_cmd, sizeof(mem_cmd)); Given the above helpers are already copying into a pointer, why not pass them out_cmd directly instead of the extra mem_cmd hop? > out_cmd->info.size_in = send_cmd->in.size; This could also be deleted since both helpers above already set size_in = send_cmd->in.size.
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index be61a0d8016b..06fbe6d079ba 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -207,6 +207,75 @@ 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; + + dev_WARN_ONCE(cxlds->dev, true, "raw command path used\n"); + + mem_cmd->info.id = CXL_MEM_COMMAND_ID_RAW; + mem_cmd->info.flags = 0; + mem_cmd->info.size_in = send_cmd->in.size; + mem_cmd->info.size_out = send_cmd->out.size; + mem_cmd->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; + + memcpy(mem_cmd, c, sizeof(*c)); + mem_cmd->info.size_in = send_cmd->in.size; + return 0; +} + /** * cxl_validate_cmd_from_user() - Check fields for CXL_MEM_SEND_COMMAND. * @cxlds: The device data for the operation @@ -230,8 +299,8 @@ 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; + struct cxl_mem_command mem_cmd; + int rc; if (send_cmd->id == 0 || send_cmd->id >= CXL_MEM_COMMAND_ID_MAX) return -ENOTTY; @@ -244,70 +313,16 @@ 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, &mem_cmd); + else + rc = cxl_to_mem_cmd(cxlds, send_cmd, &mem_cmd); - if (send_cmd->raw.rsvd) - return -EINVAL; + if (rc) + return rc; - /* - * 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)); + memcpy(out_cmd, &mem_cmd, sizeof(mem_cmd)); out_cmd->info.size_in = send_cmd->in.size; /* * XXX: out_cmd->info.size_out will be controlled by the driver, and the