Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1741154 > unrolled thread

[PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check Mario Limonciello <mario.limonciello@dell.com> - 2017-09-28 06:10 +0200
    Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI  descriptor check Darren Hart <dvhart@infradead.org> - 2017-09-30 03:30 +0200
      RE: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI  descriptor check <Mario.Limonciello@dell.com> - 2017-09-30 21:50 +0200
        Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check Pali Rohár <pali.rohar@gmail.com> - 2017-09-30 22:10 +0200
      Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI  descriptor check Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-10-01 10:50 +0200

#1741154 — [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

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(-)

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;
 
-- 
2.14.1

[toc] | [next] | [standalone]

#1742437 — 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).

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


-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1742679 — RE: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

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

[toc] | [prev] | [next] | [standalone]

#1742685

[Multipart message — attachments visible in raw view] — view raw

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)

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

[toc] | [prev] | [next] | [standalone]

#1742753 — Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

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.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web