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

Groups > linux.kernel > #1301106 > unrolled thread

[PATCH v2 0/2] Fixes for dell-wmi

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 v2 0/2] Fixes for dell-wmi Pali Rohár <pali.rohar@gmail.com> - 2016-01-04 22:30 +0100
    [PATCH v2 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid Pali Rohár <pali.rohar@gmail.com> - 2016-01-04 22:30 +0100
    [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0 Pali Rohár <pali.rohar@gmail.com> - 2016-01-04 22:30 +0100
      Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with  interface version 0 Michał Kępień <kernel@kempniu.pl> - 2016-01-12 12:20 +0100
        Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0 Pali Rohár <pali.rohar@gmail.com> - 2016-01-12 18:50 +0100
          Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with  interface version 0 Michał Kępień <kernel@kempniu.pl> - 2016-01-12 21:20 +0100
            Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with  interface version 0 Darren Hart <dvhart@infradead.org> - 2016-01-15 00:10 +0100
    Re: [PATCH v2 0/2] Fixes for dell-wmi Darren Hart <dvhart@infradead.org> - 2016-01-11 20:30 +0100
      Re: [PATCH v2 0/2] Fixes for dell-wmi Gabriele Mazzotta <gabriele.mzt@gmail.com> - 2016-01-12 01:40 +0100

#1301106 — [PATCH v2 0/2] Fixes for dell-wmi

This patch series adds check if Dell WMI descriptor structure is valid and
fixes processing WMI events on devices with WMI interface version 0.

After testing, second patch is good candidate for backporting into stable
kernels, but problem is that it cannot be used without first patch. So I
let decision to other people.

Pali Rohár (2):
  dell-wmi: Check if Dell WMI descriptor structure is valid
  dell-wmi: Process only one event on devices with interface version 0

 drivers/platform/x86/dell-wmi.c |   96 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 2 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1301109 — [PATCH v2 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

After examining existing DSDT ACPI tables of more laptops and looking
into Dell WMI document mentioned in ML dicussion archived at
http://www.spinics.net/lists/platform-driver-x86/msg07220.html we will
parse and check WMI descriptor if contains expected data. It is because
WMI descriptor contains interface version number and it is needed to
know in next commit.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   80 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 57402c4..1ad7a7b 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -2,6 +2,7 @@
  * Dell WMI hotkeys
  *
  * Copyright (C) 2008 Red Hat <mjg@redhat.com>
+ * Copyright (C) 2014-2015 Pali Rohár <pali.rohar@gmail.com>
  *
  * Portions based on wistron_btns.c:
  * Copyright (C) 2005 Miloslav Trmac <mitr@volny.cz>
@@ -38,14 +39,18 @@
 #include <acpi/video.h>
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
 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 int acpi_video;
+static u32 dell_wmi_interface_version;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
+MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
@@ -422,16 +427,87 @@ static void __init find_hk_type(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
+ */
+static int __init dell_wmi_check_descriptor_buffer(void)
+{
+	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	u32 *buffer;
+
+	status = wmi_query_block(DELL_DESCRIPTOR_GUID, 0, &out);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Cannot read Dell descriptor buffer - %d\n", status);
+		return status;
+	}
+
+	obj = (union acpi_object *)out.pointer;
+	if (!obj) {
+		pr_err("Dell descriptor buffer is empty\n");
+		return -EINVAL;
+	}
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		pr_err("Cannot read Dell descriptor buffer\n");
+		kfree(obj);
+		return -EINVAL;
+	}
+
+	if (obj->buffer.length != 128) {
+		pr_err("Dell descriptor buffer has invalid length (%d)\n",
+			obj->buffer.length);
+		if (obj->buffer.length < 16) {
+			kfree(obj);
+			return -EINVAL;
+		}
+	}
+
+	buffer = (u32 *)obj->buffer.pointer;
+
+	if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720)
+		pr_warn("Dell descriptor buffer has invalid signature (%*ph)\n",
+			8, buffer);
+
+	if (buffer[2] != 0 && buffer[2] != 1)
+		pr_warn("Dell descriptor buffer has unknown version (%d)\n",
+			buffer[2]);
+
+	if (buffer[3] != 4096)
+		pr_warn("Dell descriptor buffer has invalid buffer length (%d)\n",
+			buffer[3]);
+
+	dell_wmi_interface_version = buffer[2];
+
+	pr_info("Detected Dell WMI interface version %u\n",
+		dell_wmi_interface_version);
+
+	kfree(obj);
+	return 0;
+}
+
 static int __init dell_wmi_init(void)
 {
 	int err;
 	acpi_status status;
 
-	if (!wmi_has_guid(DELL_EVENT_GUID)) {
-		pr_warn("No known WMI GUID found\n");
+	if (!wmi_has_guid(DELL_EVENT_GUID) ||
+	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
+		pr_warn("Dell WMI GUID were not found\n");
 		return -ENODEV;
 	}
 
+	err = dell_wmi_check_descriptor_buffer();
+	if (err)
+		return err;
+
 	dmi_walk(find_hk_type, NULL);
 	acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1301112 — [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

BIOS/ACPI on devices with WMI interface version 0 does not clear buffer
before filling it. So next time when BIOS/ACPI send WMI event which is
smaller as previous then it contains garbage in buffer from previous event.

BIOS/ACPI on devices with WMI interface version 1 clears buffer and
sometimes send more events in buffer at one call.

Since commit 83fc44c32ad8 ("dell-wmi: Update code for processing WMI
events") dell-wmi process all events in buffer (and not just first).

So to prevent reading garbage from buffer we will process only first one
event on devices with WMI interface version 0.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/dell-wmi.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 1ad7a7b..5db9efb 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -237,6 +237,22 @@ static void dell_wmi_notify(u32 value, void *context)
 
 	buffer_end = buffer_entry + buffer_size;
 
+	/*
+	 * BIOS/ACPI on devices with WMI interface version 0 does not clear
+	 * buffer before filling it. So next time when BIOS/ACPI send WMI event
+	 * which is smaller as previous then it contains garbage in buffer from
+	 * previous event.
+	 *
+	 * BIOS/ACPI on devices with WMI interface version 1 clears buffer and
+	 * sometimes send more events in buffer at one call.
+	 *
+	 * So to prevent reading garbage from buffer we will process only first
+	 * one event on devices with WMI interface version 0.
+	 */
+	if (dell_wmi_interface_version == 0 && buffer_entry < buffer_end)
+		if (buffer_end > buffer_entry + buffer_entry[0] + 1)
+			buffer_end = buffer_entry + buffer_entry[0] + 1;
+
 	while (buffer_entry < buffer_end) {
 
 		len = buffer_entry[0];
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1307282 — Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

> BIOS/ACPI on devices with WMI interface version 0 does not clear buffer
> before filling it. So next time when BIOS/ACPI send WMI event which is
> smaller as previous then it contains garbage in buffer from previous event.
> 
> BIOS/ACPI on devices with WMI interface version 1 clears buffer and
> sometimes send more events in buffer at one call.
> 
> Since commit 83fc44c32ad8 ("dell-wmi: Update code for processing WMI
> events") dell-wmi process all events in buffer (and not just first).
> 
> So to prevent reading garbage from buffer we will process only first one
> event on devices with WMI interface version 0.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/platform/x86/dell-wmi.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 1ad7a7b..5db9efb 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -237,6 +237,22 @@ static void dell_wmi_notify(u32 value, void *context)
>  
>  	buffer_end = buffer_entry + buffer_size;
>  
> +	/*
> +	 * BIOS/ACPI on devices with WMI interface version 0 does not clear
> +	 * buffer before filling it. So next time when BIOS/ACPI send WMI event
> +	 * which is smaller as previous then it contains garbage in buffer from
> +	 * previous event.
> +	 *
> +	 * BIOS/ACPI on devices with WMI interface version 1 clears buffer and
> +	 * sometimes send more events in buffer at one call.
> +	 *
> +	 * So to prevent reading garbage from buffer we will process only first
> +	 * one event on devices with WMI interface version 0.
> +	 */
> +	if (dell_wmi_interface_version == 0 && buffer_entry < buffer_end)
> +		if (buffer_end > buffer_entry + buffer_entry[0] + 1)
> +			buffer_end = buffer_entry + buffer_entry[0] + 1;

Wouldn't it be a bit more clear if we clamped buffer_size before setting
buffer_end?  E.g. like this:

	if (buffer_size == 0)
		return;

	if (dell_wmi_interface_version == 0 &&
	    buffer_size > buffer_entry[0] + 1)
		buffer_size = buffer_entry[0] + 1;

	buffer_end = buffer_entry + buffer_size;

If I understand correctly, the second check on the first line added by
your patch prevents a bad dereference when accesing buffer_entry[0].
The only case when that may happen is when buffer_size is 0, which means
we got notified with rubbish anyway, so we can just return (perhaps with
a log message, which I omitted above).

One more minor nit: you should probably decide between "first" and "one"
as the phrase "only first one event" (found both in the commit message
and in the code comment) sounds incorrect to me.

-- 
Best regards,
Michał Kępień

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

#1307687 — Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

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

On Tuesday 12 January 2016 12:14:39 Michał Kępień wrote:
> > BIOS/ACPI on devices with WMI interface version 0 does not clear
> > buffer before filling it. So next time when BIOS/ACPI send WMI
> > event which is smaller as previous then it contains garbage in
> > buffer from previous event.
> > 
> > BIOS/ACPI on devices with WMI interface version 1 clears buffer and
> > sometimes send more events in buffer at one call.
> > 
> > Since commit 83fc44c32ad8 ("dell-wmi: Update code for processing
> > WMI events") dell-wmi process all events in buffer (and not just
> > first).
> > 
> > So to prevent reading garbage from buffer we will process only
> > first one event on devices with WMI interface version 0.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > 
> >  drivers/platform/x86/dell-wmi.c |   16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/dell-wmi.c
> > b/drivers/platform/x86/dell-wmi.c index 1ad7a7b..5db9efb 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -237,6 +237,22 @@ static void dell_wmi_notify(u32 value, void
> > *context)
> > 
> >  	buffer_end = buffer_entry + buffer_size;
> > 
> > +	/*
> > +	 * BIOS/ACPI on devices with WMI interface version 0 does not
> > clear +	 * buffer before filling it. So next time when BIOS/ACPI
> > send WMI event +	 * which is smaller as previous then it contains
> > garbage in buffer from +	 * previous event.
> > +	 *
> > +	 * BIOS/ACPI on devices with WMI interface version 1 clears
> > buffer and +	 * sometimes send more events in buffer at one call.
> > +	 *
> > +	 * So to prevent reading garbage from buffer we will process only
> > first +	 * one event on devices with WMI interface version 0.
> > +	 */
> > +	if (dell_wmi_interface_version == 0 && buffer_entry < buffer_end)
> > +		if (buffer_end > buffer_entry + buffer_entry[0] + 1)
> > +			buffer_end = buffer_entry + buffer_entry[0] + 1;
> 
> Wouldn't it be a bit more clear if we clamped buffer_size before
> setting buffer_end?  E.g. like this:
> 
> 	if (buffer_size == 0)
> 		return;
> 
> 	if (dell_wmi_interface_version == 0 &&
> 	    buffer_size > buffer_entry[0] + 1)
> 		buffer_size = buffer_entry[0] + 1;
> 
> 	buffer_end = buffer_entry + buffer_size;

Before return adds correct cleanup part and code will be same as my 
original patch.

So if more people think that your code is cleaner I'm OK with replacing 
it.

> If I understand correctly, the second check on the first line added
> by your patch prevents a bad dereference when accesing
> buffer_entry[0].

Yes. Same check (buffer_entry < buffer_end) is used in next whole loop, 
so I uses it in my patch too...

> The only case when that may happen is when
> buffer_size is 0,

In this one case, yes. But you can see that buffer_entry variable is 
changing (increasing pointer offset), it means that it points to some 
entry in buffer.

> which means we got notified with rubbish anyway,
> so we can just return (perhaps with a log message, which I omitted
> above).
> 
> One more minor nit: you should probably decide between "first" and
> "one" as the phrase "only first one event" (found both in the commit
> message and in the code comment) sounds incorrect to me.

Feel free to correct commit message, I'm not very good in english...

It should mean something like this... in buffer received by bios can be 
more events. That while loop iterate over events. And this my patch on 
machines with wmi version 0 will process only *one* event. And that 
event is *first* in buffer.

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

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

#1307802 — Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

> > Wouldn't it be a bit more clear if we clamped buffer_size before
> > setting buffer_end?  E.g. like this:
> > 
> > 	if (buffer_size == 0)
> > 		return;
> > 
> > 	if (dell_wmi_interface_version == 0 &&
> > 	    buffer_size > buffer_entry[0] + 1)
> > 		buffer_size = buffer_entry[0] + 1;
> > 
> > 	buffer_end = buffer_entry + buffer_size;
> 
> Before return adds correct cleanup part and code will be same as my 
> original patch.
> 
> So if more people think that your code is cleaner I'm OK with replacing 
> it.

Both solutions are fine and I realize I'm a bit late to the party as you
posted the original patch almost 3 weeks ago, so I don't want to delay
it any longer.  I think it's just a matter of deciding whether to
enforce the buffer size limit using buffer_size or buffer_end.  As the
first option involves a little bit less writing, I thought I'd suggest
it.

> > One more minor nit: you should probably decide between "first" and
> > "one" as the phrase "only first one event" (found both in the commit
> > message and in the code comment) sounds incorrect to me.
> 
> Feel free to correct commit message, I'm not very good in english...
> 
> It should mean something like this... in buffer received by bios can be 
> more events. That while loop iterate over events. And this my patch on 
> machines with wmi version 0 will process only *one* event. And that 
> event is *first* in buffer.

Don't worry, I understood your intentions from the commit message, so I
don't think it's worth posting a v3 only to correct minor stylistic
errors.

-- 
Best regards,
Michał Kępień

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

#1309749 — Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

On Tue, Jan 12, 2016 at 09:12:46PM +0100, Michał Kępień wrote:
> > > Wouldn't it be a bit more clear if we clamped buffer_size before
> > > setting buffer_end?  E.g. like this:
> > > 
> > > 	if (buffer_size == 0)
> > > 		return;
> > > 
> > > 	if (dell_wmi_interface_version == 0 &&
> > > 	    buffer_size > buffer_entry[0] + 1)
> > > 		buffer_size = buffer_entry[0] + 1;
> > > 
> > > 	buffer_end = buffer_entry + buffer_size;
> > 
> > Before return adds correct cleanup part and code will be same as my 
> > original patch.
> > 
> > So if more people think that your code is cleaner I'm OK with replacing 
> > it.
> 
> Both solutions are fine and I realize I'm a bit late to the party as you
> posted the original patch almost 3 weeks ago, so I don't want to delay
> it any longer.  I think it's just a matter of deciding whether to
> enforce the buffer size limit using buffer_size or buffer_end.  As the
> first option involves a little bit less writing, I thought I'd suggest
> it.
> 
> > > One more minor nit: you should probably decide between "first" and
> > > "one" as the phrase "only first one event" (found both in the commit
> > > message and in the code comment) sounds incorrect to me.
> > 
> > Feel free to correct commit message, I'm not very good in english...
> > 
> > It should mean something like this... in buffer received by bios can be 
> > more events. That while loop iterate over events. And this my patch on 
> > machines with wmi version 0 will process only *one* event. And that 
> > event is *first* in buffer.
> 
> Don't worry, I understood your intentions from the commit message, so I
> don't think it's worth posting a v3 only to correct minor stylistic
> errors.
> 
> -- 
> Best regards,
> Michał Kępień

I've cleaned up that bit.

-- 
Darren Hart
Intel Open Source Technology Center

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

#1306616

On Mon, Jan 04, 2016 at 10:26:34PM +0100, Pali Rohár wrote:
> This patch series adds check if Dell WMI descriptor structure is valid and
> fixes processing WMI events on devices with WMI interface version 0.
> 
> After testing, second patch is good candidate for backporting into stable
> kernels, but problem is that it cannot be used without first patch. So I
> let decision to other people.
> 
> Pali Rohár (2):
>   dell-wmi: Check if Dell WMI descriptor structure is valid
>   dell-wmi: Process only one event on devices with interface version 0
> 
>  drivers/platform/x86/dell-wmi.c |   96 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 2 deletions(-)

Thank you everyone. I have queued these to testing.

Should I add a tested-by for Michal, Andy, and Gabriele?

Do I have any Reviewed-by's to add?

-- 
Darren Hart
Intel Open Source Technology Center

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

#1306865

2016-01-11 20:22 GMT+01:00 Darren Hart <dvhart@infradead.org>:
> On Mon, Jan 04, 2016 at 10:26:34PM +0100, Pali Rohár wrote:
>> This patch series adds check if Dell WMI descriptor structure is valid and
>> fixes processing WMI events on devices with WMI interface version 0.
>>
>> After testing, second patch is good candidate for backporting into stable
>> kernels, but problem is that it cannot be used without first patch. So I
>> let decision to other people.
>>
>> Pali Rohár (2):
>>   dell-wmi: Check if Dell WMI descriptor structure is valid
>>   dell-wmi: Process only one event on devices with interface version 0
>>
>>  drivers/platform/x86/dell-wmi.c |   96 ++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 94 insertions(+), 2 deletions(-)
>
> Thank you everyone. I have queued these to testing.
>
> Should I add a tested-by for Michal, Andy, and Gabriele?

I tested both the patches, but I'm actually touched only by the
first one. So at least of that one

Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>

> Do I have any Reviewed-by's to add?
>
> --
> Darren Hart
> Intel Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web