| 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
| ||
|
Date: Wed, 1 Nov 2017 16:42:05 +0000
From: <Mario.Limonciello@...l.com>
To: <pali.rohar@...il.com>, <dvhart@...radead.org>
CC: <andy.shevchenko@...il.com>, <linux-kernel@...r.kernel.org>,
<platform-driver-x86@...r.kernel.org>, <luto@...nel.org>,
<quasisec@...gle.com>, <rjw@...ysocki.net>, <mjg59@...gle.com>,
<hch@....de>, <greg@...ah.com>, <gnomes@...rguk.ukuu.org.uk>
Subject: RE: [PATCH v11 05/15] platform/x86: dell-wmi-descriptor: split WMI
descriptor into it's own driver
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@...il.com]
> Sent: Wednesday, November 1, 2017 3:19 AM
> To: Darren Hart <dvhart@...radead.org>
> Cc: Limonciello, Mario <Mario_Limonciello@...l.com>;
> andy.shevchenko@...il.com; linux-kernel@...r.kernel.org; platform-driver-
> x86@...r.kernel.org; luto@...nel.org; quasisec@...gle.com; rjw@...ysocki.net;
> mjg59@...gle.com; hch@....de; greg@...ah.com; gnomes@...rguk.ukuu.org.uk
> Subject: Re: [PATCH v11 05/15] platform/x86: dell-wmi-descriptor: split WMI
> descriptor into it's own driver
>
> On Tuesday 31 October 2017 16:31:46 Darren Hart wrote:
> > On Mon, Oct 30, 2017 at 01:32:57PM +0000, Mario.Limonciello@...l.com wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Pali Rohár [mailto:pali.rohar@...il.com]
> > > > Sent: Monday, October 30, 2017 6:47 AM
> > > > To: Limonciello, Mario <Mario_Limonciello@...l.com>
> > > > Cc: dvhart@...radead.org; Andy Shevchenko <andy.shevchenko@...il.com>;
> > > > LKML <linux-kernel@...r.kernel.org>; platform-driver-x86@...r.kernel.org;
> Andy
> > > > Lutomirski <luto@...nel.org>; quasisec@...gle.com; rjw@...ysocki.net;
> > > > mjg59@...gle.com; hch@....de; Greg KH <greg@...ah.com>; Alan Cox
> > > > <gnomes@...rguk.ukuu.org.uk>
> > > > Subject: Re: [PATCH v11 05/15] platform/x86: dell-wmi-descriptor: split WMI
> > > > descriptor into it's own driver
> > > >
> > > > On Friday 20 October 2017 12:40:20 Mario Limonciello wrote:
> > > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.c
> > > > b/drivers/platform/x86/dell-wmi-descriptor.c
> > > > > new file mode 100644
> > > > > index 000000000000..3204c408e261
> > > > > --- /dev/null
> > > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.c
> > > >
> > > > This dell-wmi-descriptor.c looks good now!
> > > >
> > > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.h
> > > > b/drivers/platform/x86/dell-wmi-descriptor.h
> > > > > new file mode 100644
> > > > > index 000000000000..5f7b69c2c83a
> > > > > --- /dev/null
> > > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.h
> > > > > @@ -721,7 +652,9 @@ static int dell_wmi_events_set_enabled(bool
> enable)
> > > > > static int dell_wmi_probe(struct wmi_device *wdev)
> > > > > {
> > > > > struct dell_wmi_priv *priv;
> > > > > - int err;
> > > > > +
> > > > > + if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> > > > > + return -ENODEV;
> > > > >
> > > > > priv = devm_kzalloc(
> > > > > &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
> > > > > @@ -729,9 +662,8 @@ static int dell_wmi_probe(struct wmi_device
> *wdev)
> > > > > return -ENOMEM;
> > > > > dev_set_drvdata(&wdev->dev, priv);
> > > > >
> > > > > - err = dell_wmi_check_descriptor_buffer(wdev);
> > > > > - if (err)
> > > > > - return err;
> > > > > + if (!dell_wmi_get_interface_version(&priv->interface_version))
> > > > > + return -EPROBE_DEFER;
> > > >
> > > > But here is still a problem. You added check that
> > > > DELL_WMI_DESCRIPTOR_GUID exists in APCI table, but it does not mean that
> > > > probe method of dell-wmi-descriptor cannot fail.
> > > >
> > > > With PROBE_DEFER, dell_wmi_probe function would be called later again
> > > > and again, even when probing dell-wmi-descriptor failed and so dell-wmi
> > > > in this case cannot work.
> > > >
> > >
> > > Yes it's possible that probe method can fail, but it depends on the reason for
> > > failure if it will fail again later. For example if not enough memory, it may work
> > > later. Or maybe user manually unbound from GUID, should continue to try until
> > > it's bound again.
> > >
> > > So in short, I believe this is the correct behavior to adopt.
> >
> > In this case, I believe the synchronous request_module("dell-wmi-descriptor")
> > would provide the desired result. The exit status doesn't even need to be
> > checked. If that is successful, and the interface_version still returns false,
> > then it can be considered an error and we can exit. If it fails, the
> > interface_version will return false, and it is also an error.
> >
> > This can be easily added as a single patch on top of this series:
> >
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index dcfa5de..964ca54 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -665,8 +665,9 @@ static int dell_wmi_probe(struct wmi_device *wdev)
> > return -ENOMEM;
> > dev_set_drvdata(&wdev->dev, priv);
> >
> > + request_module("dell-wmi-descriptor");
> > if (!dell_wmi_get_interface_version(&priv->interface_version))
> > - return -EPROBE_DEFER;
> > + return -ENODEV;
> >
> > return dell_wmi_input_setup(wdev);
> > }
> >
> > Pali, I believe this addresses your concern?
This proposed approach would actually introduce a problem of when a device
is unbound on the dell-wmi-descriptor then you bind dell-wmi now it's
possible to get into a state of probe routine failed on dell-wmi and can't be
fixed without unbind dell-wmi, bind dell-wmi-descriptor and then rebind
dell-wmi.
With deferred probing solution I had in place that above problem can't happen
because probing would just re-run.
>
> I'm not sure what happen if both drivers are statically linked into
> vmlinuz and dell-wmi probe method would be called before
> dell-wmi-descriptor method.
That's why this is better to do using deferred probing I think still.
>
> What is doing request_module when requested module is statically linked
> into vmlinuz?
>
As I can tell request_module actually calls out to userspace to run modprobe
on the requested module.
Something that may help with Pali's concern is to in the probe routine for
dell-wmi-descritptor set something in a global variable for an error code and
have a function to look that up. In the error scenario for a non memory
allocation error set that value. Something like this:
/* valid = 0 means probe hasn't run, valid = 1 means successful probe,
* valid < 0 means failed probe due to bad device
*/
static int dell_wmi_descriptor_valid(void)
{
int ret;
mutex_lock(&list_mutex);
if (valid == 0)
ret = -EPROBE_DEFER;
else if
ret = valid;
mutex_unlock(&list_mutex)
return ret;
}
Then dell-wmi and dell-smbios-wmi can call that before dell_wmi_get_interface_version.
Does that satisfy Pali's concern? I may follow up with this particular item in a later patch
so as to not derail current efforts unless Darren or Andy would like this sorted ASAP or I
get through testing it quickly.
Powered by blists - more mailing lists