On 21-02-19 20:22:00, Konrad Rzeszutek Wilk wrote:
> ..snip..
> > +static int handle_mailbox_cmd_from_user(struct cxl_mem *cxlm,
> > + const struct cxl_mem_command *cmd,
> > + u64 in_payload, u64 out_payload,
> > + s32 *size_out, u32 *retval)
> > +{
> > + struct device *dev = &cxlm->pdev->dev;
> > + struct mbox_cmd mbox_cmd = {
> > + .opcode = cmd->opcode,
> > + .size_in = cmd->info.size_in,
> > + .size_out = cmd->info.size_out,
> > + };
> > + int rc;
> > +
> > + if (cmd->info.size_out) {
> > + mbox_cmd.payload_out = kvzalloc(cmd->info.size_out, GFP_KERNEL);
> > + if (!mbox_cmd.payload_out)
> > + return -ENOMEM;
> > + }
> > +
> > + if (cmd->info.size_in) {
> > + mbox_cmd.payload_in = vmemdup_user(u64_to_user_ptr(in_payload),
> > + cmd->info.size_in);
> > + if (IS_ERR(mbox_cmd.payload_in))
> > + return PTR_ERR(mbox_cmd.payload_in);
>
> Not that this should happen, but what if info.size_out was set? Should
> you also free mbox_cmd.payload_out?
>
Thanks Konrad.
Dan, do you want me to send a fixup patch? This bug was introduced from v4->v5.
> > + }
> > +
> > + rc = cxl_mem_mbox_get(cxlm);
> > + if (rc)
> > + goto out;
> > +
> > + dev_dbg(dev,
> > + "Submitting %s command for user\n"
> > + "\topcode: %x\n"
> > + "\tsize: %ub\n",
> > + cxl_command_names[cmd->info.id].name, mbox_cmd.opcode,
> > + cmd->info.size_in);
> > +
> > + rc = __cxl_mem_mbox_send_cmd(cxlm, &mbox_cmd);
> > + cxl_mem_mbox_put(cxlm);
> > + if (rc)
> > + goto out;
> > +
> > + /*
> > + * @size_out contains the max size that's allowed to be written back out
> > + * to userspace. While the payload may have written more output than
> > + * this it will have to be ignored.
> > + */
> > + if (mbox_cmd.size_out) {
> > + dev_WARN_ONCE(dev, mbox_cmd.size_out > *size_out,
> > + "Invalid return size\n");
> > + if (copy_to_user(u64_to_user_ptr(out_payload),
> > + mbox_cmd.payload_out, mbox_cmd.size_out)) {
> > + rc = -EFAULT;
> > + goto out;
> > + }
> > + }
> > +
> > + *size_out = mbox_cmd.size_out;
> > + *retval = mbox_cmd.return_code;
> > +
> > +out:
> > + kvfree(mbox_cmd.payload_in);
> > + kvfree(mbox_cmd.payload_out);
> > + return rc;
> > +}
>
> ..snip..
>
> > +static int cxl_query_cmd(struct cxl_memdev *cxlmd,
> > + struct cxl_mem_query_commands __user *q)
> > +{
> > + struct device *dev = &cxlmd->dev;
> > + struct cxl_mem_command *cmd;
> > + u32 n_commands;
> > + int j = 0;
>
> How come it is 'j' instead of the usual 'i'?
Just how it got split out/copied from an earlier version of the series.
I think rename to i, or cmds would be best as well. I/Dan can do it as part of
the bug fix you found above.
> > +
> > + dev_dbg(dev, "Query IOCTL\n");
> > +
> > + if (get_user(n_commands, &q->n_commands))
> > + return -EFAULT;
> > +
> > + /* returns the total number if 0 elements are requested. */
> > + if (n_commands == 0)
> > + return put_user(cxl_cmd_count, &q->n_commands);
> > +
> > + /*
> > + * otherwise, return max(n_commands, total commands) cxl_command_info
> > + * structures.
> > + */
> > + cxl_for_each_cmd(cmd) {
> > + const struct cxl_command_info *info = &cmd->info;
> > +
> > + if (copy_to_user(&q->commands[j++], info, sizeof(*info)))
> > + return -EFAULT;
> > +
> > + if (j == n_commands)
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
