Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jarkko Sakkinen]

On Sun, Oct 09, 2016 at 10:49:05AM -0600, Jason Gunthorpe wrote:
> On Sun, Oct 09, 2016 at 12:38:18PM +0300, Jarkko Sakkinen wrote:
> > On Sat, Oct 08, 2016 at 07:42:56PM -0600, Jason Gunthorpe wrote:
> > > On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote:
> > > > +	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
> > > > +			   sizeof(struct crb_regs) -
> > > > +			   offsetof(struct crb_regs, ctrl_req));
> > > > +	if (IS_ERR(ctrl))
> > > > +		return PTR_ERR(ctrl);
> > > > +
> > > > +	/* The control area always overrlaps IO memory mapped from the ACPI
> > > > +	 * object with CRB start only devices. Thus, this is perfectly safe.
> > > > +	 */
> > > > +	priv->regs = (void *)((unsigned long)ctrl -
> > > > +		offsetof(struct crb_regs, ctrl_req));
> > > 
> > > Hum. No, this makes bad assumptions about the structure of iomapping.
> > > 
> > > The map itself needs to be done with the adjustment:
> > > 
> > > 	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address -
> > > 		offsetof(struct crb_regs, ctrl_req),
> > > 	   	sizeof(struct crb_regs));
> > 
> > That would be wrong address for the control area as it does not start
> > from the beginning of CRB registers.
> 
> Of course, I just pointed out what the map call should look like
> 
> Something like this
> 
>  	priv->regs = crb_map_res(dev, priv, &io_res, buf->control_address -
>  		offsetof(struct crb_regs, ctrl_req),
>  	   	sizeof(struct crb_regs));
>         ctrl = &priv->regs.ctrl_req;

Sorry I missed this part.

Here are the constraints for existing hardware:

1. All the existing CRB start only hardware has the iomem covering the
   control area and registers for multiple localities.
2. All the existing ACPI start hardware has only the control area.

If you assume that SSDT does not have malicous behavior caused by either
a BIOS bug or maybe a rootkit, then the current patch works for all the
existing hardware.

To counter-measure for unexpected behavior in non-existing hardware and
buggy or malicious firmware it probably make sense to use crb_map_res to
validate the part of the CRB registers that is not part of the control
area.

Doing it in the way you proposed does not work for ACPI start devices.

For them it should be done in the same way as I'm doing in the existing
patch as for ACPI start devices the address below the control area are
never accessed. Having a separate crb_map_res for CRB start only devices
is sane thing to do for validation.

/Jarkko