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

[Mario.Limonciello]

> -----Original Message-----
> From: Pali RohÃr [mailto:pali.rohar@xxxxxxxxx]
> Sent: Friday, September 29, 2017 2:36 AM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: dvhart@xxxxxxxxxxxxx; andy.shevchenko@xxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; luto@xxxxxxxxxx;
> quasisec@xxxxxxxxxx
> 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@xxxxxxxx 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?

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.
  
> 
> > > Devices from the bus subsystem should not use global shared variables,
> > > but rather allocate own memory...
> >
> > Part of the problem is that this module can operate in two modes now.
> > I think there will have to be global shared variables when operating in SMI
> > mode.  Or maybe put those bits into a platform_device for SMI mode usage?
> >
> > The problem I think then becomes how do you handle
> dell_smbios_send_request?
> > Without global shared memory for the module the dell-laptop and dell-wmi
> > modules won't be able to use this.
> 
> 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.

> 
> > > Also note that user can at any time unbound device from driver and also
> > > can bind it again.
> > I'll make some adjustments to account for this.
> 
> To prevent crashing kernel, this needs to be written correctly. And user
> should not be able to crash kernel just when trying to unbound driver
> from the device.
> 

Agree.