Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

["Pali Rohár" <pali.rohar@gmail.com>]

On Monday 02 October 2017 14:15:11 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Saturday, September 30, 2017 3:01 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux-
> > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org;
> > quasisec@google.com
> > Subject: Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI
> > descriptor check
> > 
> > On Saturday 30 September 2017 21:48:39 Mario.Limonciello@dell.com wrote:
> > > > > +/*
> > > >
> > > > > + * Descriptor buffer is 128 byte long and contains:
> > > > ...
> > > >
> > > > > +	if (obj->buffer.length != 128) {
> > > > > +		dev_err(&wdev->dev,
> > > > > +			"Dell descriptor buffer has invalid length (%d)\n",
> > > > > +			obj->buffer.length);
> > > >
> > > > This seems odd. We call it an error (not a warning) if != 128, but
> > > > we only abort and return an error if it's < 16.
> > > >
> > > > If it's an error, we should return an error code, if anything above
> > > > 16 is acceptable but 128 is preferred, the above should be a
> > > > warning at best. (this scenario seems unlikely).
> > >
> > > Hopefully the original author can speak up to the intentions here.  I
> > > would feel that it should have errored out if it wasn't expected
> > > length too.
> > 
> > Code below access first 16 bytes of buffer. Therefore to prevent buffer
> > overflow check for 16 bytes is needed.
> > 
> > But IIRC we decided to do not throw error and continue driver loading
> > even when buffer length is not 128 (as expected by some Dell
> > documentation) as it could be possible regression because driver itself
> > does not depend on buffer length.
> > 
> 
> So I'm intending to change this in my next patch series.  I feel it should throw an
> error when the buffer length isn't 128.
> 
> My logic is that if you don't see the proper buffer size (or the proper header)
> then how can you trust that the rest of the data is reliable?  This means the format
> has changed or this isn't a real descriptor as expected by Dell (say some other vendor
> that has cloned the GUID).  It's better to abort in this situation.

Error handling now is up to you -- Dell. You know the best how your
API/ABI behave.

I did that change to be fully backward compatible with possibility to
read interface version number (needed for event handling logic).

> > > > > +		if (obj->buffer.length < 16) {
> > > > > +			ret = -EINVAL;
> > > > > +			goto out;
> > > > > +		}
> > > > > +	}
> > > > > +	desc_buffer = (u32 *)obj->buffer.pointer;
> > > > > +
> > > > > +	if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] !=
> > > > > 0x494D5720)
> > 
> > --
> > Pali Rohár
> > pali.rohar@gmail.com

-- 
Pali Rohár
pali.rohar@gmail.com