| Message ID | bea602b33e27facc77ee850ef14dfa37f6894275.1506571188.git.mario.limonciello@dell.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Wed, Sep 27, 2017 at 11:02:15PM -0500, Mario Limonciello wrote: > The Dell WMI descriptor check is used as an indication that WMI > calls are safe to run both when used with the notification > ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair. > > As some code in dell-wmi-smbios is already a prerequisite for > dell-wmi, move the code for performing the descriptor check into > dell-wmi-smbios and let both drivers use it from there. > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > --- ... > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c ... > > +/* > + * 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). > + if (obj->buffer.length < 16) { > + ret = -EINVAL; > + goto out; > + } > + } > + desc_buffer = (u32 *)obj->buffer.pointer; > + > + if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720) This seems like it should be an || ? (I see this is fixed in a later patch) I would have suggested fixing it, then moving it - just from a pure ease of review perspective. Perhaps not in the comment above that this is a verbatim move, some fixes are coming in subsequent patches. This is worth noting for a future develop who may be choosing what to backport. > static int dell_smbios_wmi_probe(struct wmi_device *wdev) > { > + int ret; > + u32 interface_version; Declarations in order of decreasing line length please. > @@ -236,6 +316,7 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev) > if (!buffer) > return -ENOMEM; > bufferlen = sizeof(struct wmi_calling_interface_buffer); > + Stray whitespace, should have been in the previous patch I guess?
> -----Original Message----- > From: Darren Hart [mailto:dvhart@infradead.org] > Sent: Friday, September 29, 2017 8:29 PM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; LKML <linux- > kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; Andy Lutomirski > <luto@kernel.org>; quasisec@google.com; pali.rohar@gmail.com > Subject: Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI > descriptor check > > On Wed, Sep 27, 2017 at 11:02:15PM -0500, Mario Limonciello wrote: > > The Dell WMI descriptor check is used as an indication that WMI > > calls are safe to run both when used with the notification > > ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair. > > > > As some code in dell-wmi-smbios is already a prerequisite for > > dell-wmi, move the code for performing the descriptor check into > > dell-wmi-smbios and let both drivers use it from there. > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > > --- > ... > > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell- > smbios.c > ... > > > > +/* > > + * 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. > > > + if (obj->buffer.length < 16) { > > + ret = -EINVAL; > > + goto out; > > + } > > + } > > + desc_buffer = (u32 *)obj->buffer.pointer; > > + > > + if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720) > > This seems like it should be an || ? > > (I see this is fixed in a later patch) > > I would have suggested fixing it, then moving it - just from a pure ease of > review perspective. Perhaps not in the comment above that this is a verbatim > move, some fixes are coming in subsequent patches. This is worth noting for a > future develop who may be choosing what to backport. > I'll re-order things so that the fixes come before the move. > > static int dell_smbios_wmi_probe(struct wmi_device *wdev) > > { > > + int ret; > > + u32 interface_version; > > Declarations in order of decreasing line length please. > > > @@ -236,6 +316,7 @@ static int dell_smbios_wmi_probe(struct wmi_device > *wdev) > > if (!buffer) > > return -ENOMEM; > > bufferlen = sizeof(struct wmi_calling_interface_buffer); > > + > > Stray whitespace, should have been in the previous patch I guess? > > > -- > Darren Hart > VMware Open Source Technology Center
On Saturday 30 September 2017 21:48:39 Mario.Limonciello@dell.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. > > > + if (obj->buffer.length < 16) { > > > + ret = -EINVAL; > > > + goto out; > > > + } > > > + } > > > + desc_buffer = (u32 *)obj->buffer.pointer; > > > + > > > + if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != > > > 0x494D5720)
On Sat, Sep 30, 2017 at 4:29 AM, Darren Hart <dvhart@infradead.org> wrote: > On Wed, Sep 27, 2017 at 11:02:15PM -0500, Mario Limonciello wrote: >> The Dell WMI descriptor check is used as an indication that WMI >> calls are safe to run both when used with the notification >> ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair. >> >> As some code in dell-wmi-smbios is already a prerequisite for >> dell-wmi, move the code for performing the descriptor check into >> dell-wmi-smbios and let both drivers use it from there. >> + if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720) > > This seems like it should be an || ? > > (I see this is fixed in a later patch) Interesting, I proposed to use strncmp(), but it's gone again. Perhaps I missed what is downside of that.
> -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Saturday, September 30, 2017 3:01 PM > 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 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI > descriptor check > > On Saturday 30 September 2017 21:48:39 Mario.Limonciello@dell.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@gmail.com
On Monday 02 October 2017 14:15:11 Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Pali Rohár [mailto:pali.rohar@gmail.com] > > Sent: Saturday, September 30, 2017 3:01 PM > > 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 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI > > descriptor check > > > > On Saturday 30 September 2017 21:48:39 Mario.Limonciello@dell.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@gmail.com
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c index 4472817ee045..5d793b012e5e 100644 --- a/drivers/platform/x86/dell-smbios.c +++ b/drivers/platform/x86/dell-smbios.c @@ -31,6 +31,7 @@ #endif #define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492" +#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492" struct calling_interface_structure { struct dmi_header header; @@ -226,8 +227,87 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy) } } +/* + * Descriptor buffer is 128 byte long and contains: + * + * Name Offset Length Value + * Vendor Signature 0 4 "DELL" + * Object Signature 4 4 " WMI" + * WMI Interface Version 8 4 <version> + * WMI buffer length 12 4 4096 + */ +int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version) +{ + union acpi_object *obj = NULL; + struct wmi_device *desc_dev; + u32 *desc_buffer; + int ret; + + desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID); + if (!desc_dev) { + dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n"); + return -ENODEV; + } + + obj = wmidev_block_query(desc_dev, 0); + if (!obj) { + dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n"); + ret = -EIO; + goto out; + } + + if (obj->type != ACPI_TYPE_BUFFER) { + dev_err(&wdev->dev, "Dell descriptor has wrong type\n"); + ret = -EINVAL; + goto out; + } + + if (obj->buffer.length != 128) { + dev_err(&wdev->dev, + "Dell descriptor buffer has invalid length (%d)\n", + obj->buffer.length); + if (obj->buffer.length < 16) { + ret = -EINVAL; + goto out; + } + } + desc_buffer = (u32 *)obj->buffer.pointer; + + if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720) + dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n", + 8, desc_buffer); + + if (desc_buffer[2] != 0 && desc_buffer[2] != 1) + dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n", + desc_buffer[2]); + + if (desc_buffer[3] != 4096) + dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n", + desc_buffer[3]); + + *version = desc_buffer[2]; + ret = 0; + + dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n", + *version); + +out: + kfree(obj); + put_device(&desc_dev->dev); + return ret; +} +EXPORT_SYMBOL_GPL(dell_wmi_check_descriptor_buffer); + + static int dell_smbios_wmi_probe(struct wmi_device *wdev) { + int ret; + u32 interface_version; + + ret = dell_wmi_check_descriptor_buffer(wdev, &interface_version); + if (ret) + return -ENODEV; + /* no longer need the SMI page */ free_page((unsigned long)buffer); @@ -236,6 +316,7 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev) if (!buffer) return -ENOMEM; bufferlen = sizeof(struct wmi_calling_interface_buffer); + wmi_dev = wdev; return 0; } diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h index 2f6fce81ee69..be9ec1ccf8bf 100644 --- a/drivers/platform/x86/dell-smbios.h +++ b/drivers/platform/x86/dell-smbios.h @@ -17,6 +17,8 @@ #ifndef _DELL_SMBIOS_H_ #define _DELL_SMBIOS_H_ +#include <linux/wmi.h> + struct notifier_block; /* If called through fallback SMI rather than WMI this structure will be @@ -62,5 +64,6 @@ enum dell_laptop_notifier_actions { int dell_laptop_register_notifier(struct notifier_block *nb); int dell_laptop_unregister_notifier(struct notifier_block *nb); void dell_laptop_call_notifier(unsigned long action, void *data); +int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version); #endif diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c index 1fbef560ca67..b3bb8cdd27bf 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -46,7 +46,6 @@ MODULE_DESCRIPTION("Dell laptop WMI hotkeys driver"); MODULE_LICENSE("GPL"); #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492" -#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492" static bool wmi_requires_smbios_request; @@ -617,78 +616,6 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev) input_unregister_device(priv->input_dev); } -/* - * Descriptor buffer is 128 byte long and contains: - * - * Name Offset Length Value - * Vendor Signature 0 4 "DELL" - * Object Signature 4 4 " WMI" - * WMI Interface Version 8 4 <version> - * WMI buffer length 12 4 4096 - */ -static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev) -{ - struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev); - union acpi_object *obj = NULL; - struct wmi_device *desc_dev; - u32 *buffer; - int ret; - - desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID); - if (!desc_dev) { - dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n"); - return -ENODEV; - } - - obj = wmidev_block_query(desc_dev, 0); - if (!obj) { - dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n"); - ret = -EIO; - goto out; - } - - if (obj->type != ACPI_TYPE_BUFFER) { - dev_err(&wdev->dev, "Dell descriptor has wrong type\n"); - ret = -EINVAL; - goto out; - } - - if (obj->buffer.length != 128) { - dev_err(&wdev->dev, - "Dell descriptor buffer has invalid length (%d)\n", - obj->buffer.length); - if (obj->buffer.length < 16) { - ret = -EINVAL; - goto out; - } - } - - buffer = (u32 *)obj->buffer.pointer; - - if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720) - dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n", - 8, buffer); - - if (buffer[2] != 0 && buffer[2] != 1) - dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n", - buffer[2]); - - if (buffer[3] != 4096) - dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n", - buffer[3]); - - priv->interface_version = buffer[2]; - ret = 0; - - dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n", - priv->interface_version); - -out: - kfree(obj); - put_device(&desc_dev->dev); - return ret; -} - /* * According to Dell SMBIOS documentation: * @@ -732,7 +659,7 @@ static int dell_wmi_probe(struct wmi_device *wdev) return -ENOMEM; dev_set_drvdata(&wdev->dev, priv); - err = dell_wmi_check_descriptor_buffer(wdev); + err = dell_wmi_check_descriptor_buffer(wdev, &priv->interface_version); if (err) return err;
The Dell WMI descriptor check is used as an indication that WMI calls are safe to run both when used with the notification ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair. As some code in dell-wmi-smbios is already a prerequisite for dell-wmi, move the code for performing the descriptor check into dell-wmi-smbios and let both drivers use it from there. Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> --- drivers/platform/x86/dell-smbios.c | 81 ++++++++++++++++++++++++++++++++++++++ drivers/platform/x86/dell-smbios.h | 3 ++ drivers/platform/x86/dell-wmi.c | 75 +---------------------------------- 3 files changed, 85 insertions(+), 74 deletions(-)