On 21-01-14 18:08:26, Jonathan Cameron wrote:
> On Mon, 11 Jan 2021 14:51:20 -0800
> Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote:
>
> > The Get Log command returns the actual log entries that are advertised
> > via the Get Supported Logs command (0400h). CXL device logs are selected
> > by UUID which is part of the CXL spec. Because the driver tries to
> > sanitize what is sent to hardware, there becomes a need to restrict the
> > types of logs which can be accessed by userspace. For example, the
> > vendor specific log might only be consumable by proprietary, or offline
> > applications, and therefore a good candidate for userspace.
> >
> > The current driver infrastructure does allow basic validation for all
> > commands, but doesn't inspect any of the payload data. Along with Get
> > Log support comes new infrastructure to add a hook for payload
> > validation. This infrastructure is used to filter out the CEL UUID,
> > which the userspace driver doesn't have business knowing, and taints on
> > invalid UUIDs being sent to hardware.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@xxxxxxxxx>
>
> Just a minor question for this one.
>
> Thanks, J
> ... \
> > @@ -515,6 +529,15 @@ static int handle_mailbox_cmd_from_user(struct cxl_memdev *cxlmd,
> > int rc;
> >
> > if (cmd->info.size_in) {
> > + if (cmd->validate_payload) {
> > + rc = cmd->validate_payload(u64_to_user_ptr(in_payload),
> > + cmd->info.size_in);
>
> Is it worth moving this out of the region in which we hold the mbox?
> (after fixing the bug that I think means we don't actually hold it at this point)
>
> Perhaps not, but it does feel odd to do validation under the lock.
>
>
When moving to a bounce buffer the locking resolves itself and ultimately this
doesn't happen under lock anymore.
if (cmd->info.size_in) {
if (cmd->validate_payload) {
rc = cmd->validate_payload(u64_to_user_ptr(in_payload),
cmd->info.size_in);
if (rc)
goto out;
}
mbox_cmd.payload_in = kvzalloc(cmd->info.size_in, GFP_KERNEL);
if (!mbox_cmd.payload_in) {
rc = -ENOMEM;
goto out;
}
if (copy_from_user(mbox_cmd.payload_in,
u64_to_user_ptr(in_payload),
cmd->info.size_in)) {
rc = -EFAULT;
goto out;
}
}
rc = cxl_mem_mbox_get(cxlm, true);
> > + if (rc) {
> > + cxl_mem_mbox_put(cxlmd->cxlm);
> > + return -EFAULT;
> > + }
> > + }
> > +
> > /*
> > * Directly copy the userspace payload into the hardware. UAPI
> > * states that the buffer must already be little endian.
