Re: [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO

[Alison Schofield <alison.schofield@intel.com>]

Thanks for the review Ben...

On Tue, Jan 04, 2022 at 03:21:34PM -0800, Ben Widawsky wrote:
> On 22-01-03 12:21:00, alison.schofield@intel.com wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > User space may send the SET_PARTITION_INFO mailbox command using
> > the IOCTL interface. Inspect the input payload and fail if the
> > immediate flag is set.
> > 
> > This is the first instance of the driver inspecting an input payload
> > from user space. Assume there will be more such cases and implement
> > with an extensible helper.
> 
> Not sure if it's useful, but this was implemented at some point:
> https://lore.kernel.org/linux-cxl/20210210000259.635748-8-ben.widawsky@intel.com/

Thanks. Looking.

> 
snip

> > @@ -405,6 +440,14 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
> >  		}
> >  	}
> >  
> > +	if (!cxl_payload_from_user_allowed(mbox_cmd.opcode,
> > +					   mbox_cmd.payload_in)) {
> > +		dev_dbg(dev, "%s: input payload not allowed\n",
> > +			cxl_command_names[cmd->info.id].name);
> > +		rc = -EINVAL;
> > +		goto out;
> > +	}
> > +
> 
> Perhaps foolishly, the kdocs for handle_mailbox_cmd_from_user() documents the
> error conditions. Would you mind adding EINVAL?

I'm going to try to mv the allowable check, so this will go away.

> 
> Also, cxl_validate_cmd_from_user() was supposed to handle this kind of stuff.
> All validation from user commands should spawn from that. Is there some reason
> this one is different?

The existing cxl_validate_cmd_from_user() happens before we copy the
payload from user - so this payload check didn't fit in there.
Let me look at reorganizing a bit so validating the cmd and it's payload
can both spawn from cxl_validate_cmd_from_user().

> 

snip

>
> > +#define  CXL_SET_PARTITION_IMMEDIATE_FLAG	BIT(0)
> > +
> 
> I think these defines belong in cxl.h
> 
It seems this is OK in cxl.h where other CXL spec defines now live.

> >  /**
> >   * struct cxl_mem_command - Driver representation of a memory device command
> >   * @info: Command information as it exists for the UAPI
> > 
> > base-commit: 53989fad1286e652ea3655ae3367ba698da8d2ff
> > -- 
> > 2.31.1
> >