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 for Android: free password hash cracker in your pocket

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

Message-ID: <42642a2d212b493db280c7bac6d50564@ausx13mpc120.AMER.DELL.COM>
Date:   Mon, 2 Oct 2017 14:15:11 +0000
From:   <Mario.Limonciello@...l.com>
To:     <pali.rohar@...il.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

> -----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.

> > > > +		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