RE: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support

[Ioana Ciornei]

> Subject: Re: [PATCH v3 2/4] bus: fsl-mc: add fsl-mc userspace support
> 
> On Tue, Nov 20, 2018 at 05:59:26PM +0000, Ioana Ciornei wrote:
> > > > +static int fsl_mc_uapi_send_command(unsigned long arg,
> > > > +				    struct fsl_mc_io *mc_io)
> > > > +{
> > > > +	struct fsl_mc_command mc_cmd;
> > > > +	int error;
> > > > +
> > > > +	error = copy_from_user(&mc_cmd, (void __user *)arg, sizeof(mc_cmd));
> > > > +	if (error)
> > > > +		return -EFAULT;
> > > > +
> > > > +	error = mc_send_command(mc_io, &mc_cmd);
> > > > +	if (error)
> > > > +		return error;
> > > > +
> > > > +	error = copy_to_user((void __user *)arg, &mc_cmd, sizeof(mc_cmd));
> > > > +	if (error)
> > > > +		return -EFAULT;
> > > > +
> > > > +	return 0;
> > > > +}
> > >
> > > I know you said that "the firmware will properly verify the command"
> > > already, but given that I used to be a firmware developer a long
> > > time ago, I can almost guarantee that this will cause problems in the future.
> > >
> > > Want to make a friendly bet about this?
> > >
> > > What is the odds that your firmware api/interface has been properly
> > > fuzzed such that all possible combinations of bad commands will
> > > really not do horrible things to the hardware/system?
> > >
> > > Are you all willing to bet the system intregrity on this?  If so,
> > > ok, it's your systems
> > > :)
> > >
> > > Personally, I think you need to add a "known whitelist" and do some
> > > sort of sanity checking here.
> > >
> >
> > I can add a whitelist on the command ids that can be received from
> > userspace but leave the parameter parsing to the firmware to process
> > and interpret depending on the current system settings.
> >
> > Would that be a viable option from your point of view?
> 
> That might be a good start.  Again, how well have you tested this firmware
> interface can handle invalid data?  Are you willing to bet your system integrity on
> it?  :)
> 

We understand your concerns regarding the integrity of the MC firmware.

Each command has validation checks inside the MC and we are currently doing extensive testing with fuzzed commands from user space.
We'll come back with a v4 on this once our tests are complete and we are confident our firmware is robust enough.

Ioana C