Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

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

[-- Attachment #1: Type: Text/Plain, Size: 5181 bytes --]

On Saturday 30 September 2017 22:01:04 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Friday, September 29, 2017 2:36 AM
> > 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 2/8] platform/x86: dell-smbios: Introduce a
> > WMI-ACPI interface
> > 
> > On Thursday 28 September 2017 22:43:36 Mario.Limonciello@dell.com
> > wrote:
> > > > > @@ -170,10 +226,43 @@ static void __init find_tokens(const
> > > > > struct
> > 
> > dmi_header
> > 
> > > > *dm, void *dummy)
> > > > 
> > > > >  	}
> > > > >  
> > > > >  }
> > > > > 
> > > > > -static int __init dell_smbios_init(void)
> > > > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> > > > > +{
> > > > > +	/* no longer need the SMI page */
> > > > > +	free_page((unsigned long)buffer);
> > > > > +
> > > > > +	/* WMI buffer should be 32k */
> > > > > +	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
> > > > > +	if (!buffer)
> > > > > +		return -ENOMEM;
> > > > > +	bufferlen = sizeof(struct wmi_calling_interface_buffer);
> > > > > +	wmi_dev = wdev;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int dell_smbios_wmi_remove(struct wmi_device *wdev)
> > > > > 
> > > > >  {
> > > > > 
> > > > > -	int ret;
> > > > > +	wmi_dev = NULL;
> > > > > +	free_pages((unsigned long)buffer, 3);
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > This code does not seem to be safe. dell_smbios_wmi_probe and
> > > > dell_smbios_wmi_remove are called at any time when kernel
> > > > register new device which matches some properties OR when user
> > > > manually bind this driver to that device.
> > > 
> > > I'll adjust for the assumptions I made about it only happening at
> > > module init.
> > > 
> > > > buffer and wmi_dev is shared as a global variable which means
> > > > that when there are two devices which want to bind to this
> > > > driver, kernel would get double free at removing time.
> > > 
> > > But there is only one GUID in id_table.
> > 
> > That is truth, but ...
> > 
> > > How can two devices bind to this?
> > > This should be an impossible scenario.
> > 
> > ... in ACPI DSDT can be more WMI _WDG buffers which could lead to
> > more wmi buses and each could have own GUID. Therefore there is a
> > theoretical chance that specially prepared ACPI DSDT code can
> > cause this problem.
> 
> I guess I'm a bit confused - is this for protecting a user who
> patches their own DSDT or from a vendor releasing a system with two
> _WDG buffers with the same GUID?

It is protection for memory corruption done by dell-smbios driver in 
case such DSDT would be parsed by kernel (either by user who patches 
original DSDT or when vendor created such DSDT by mistake or by 
malicious software which patch/provide such DSDT...).

DSDT is from kernel point of view external (possible untrusted) data. 
ACPI parser in kernel parse it if they are syntactically correct, but 
semantic or e.g. _WDG meaning is up to consumer -- in our case wmi and 
dell-smbios wmi drivers.

> I don't believe MOF has a concept of which WDG buffer GUIDs are
> assigned to.  That could lead to massively undefined behavior too
> since the WDG buffer will potentially assign different ASL to
> process for each GUID. 

Yes, that would lead to undefined behaviour. But still it should not 
crash kernel. Either treat such thing as "corrupted data" and refuse to 
use it or treat it in deterministic way, e.g. "first come, first serve" 
or load it driver for all guids...

E.g. DSDT on Thinkpads has more _WDG buffers and I was told that some 
machines with Nvidia graphics cards really have duplicate WMI GUIDs in 
_WDG. So such situation really happen in world, it is not just 
theoretical.

> > The whole problem is that SMBIOS calls are implemented via
> > singleton pattern. And this singleton "instance" needs to use
> > something which is not a singleton, but a dynamic objects (wmi
> > device <--> driver pattern).
> > 
> > Idea how to handle it:
> > * put wmi smbios call function into own driver
> > * put smm smbios call function into own driver
> > * create dispatcher function which take first available device of
> > one of
> > 
> >   the above driver and call on them smbios call function
> > 
> > This problem is very similar to problems in objects world... driver
> > as a class and device as a instance.
> > 
> > (Or if somebody has a better idea, let us know...)
> 
> So I like the 3rd idea the most, and it's what I'm working on.  I've
> got some problems with it that I'm still fixing, but unless someone
> tells me otherwise I'll go for that with v4.

All above 3 points (*) were meant as one solution. Putting wmi and smm 
calls into own drivers and then creating dispatcher function which would 
use available device of one of those two drivers.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]