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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 2 Oct 2017 16:37:54 +0200
From:   Pali Rohár <pali.rohar@...il.com>
To:     Mario.Limonciello@...l.com
Cc:     dvhart@...radead.org, andy.shevchenko@...il.com,
        linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        luto@...nel.org, quasisec@...gle.com
Subject: Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI
 descriptor check

On Monday 02 October 2017 14:15:11 Mario.Limonciello@...l.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@...il.com]
> > Sent: Saturday, September 30, 2017 3:01 PM
> > To: Limonciello, Mario <Mario_Limonciello@...l.com>
> > Cc: dvhart@...radead.org; andy.shevchenko@...il.com; linux-
> > kernel@...r.kernel.org; platform-driver-x86@...r.kernel.org; luto@...nel.org;
> > quasisec@...gle.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@...l.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@...il.com

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