| Message ID | 1451942796-26574-3-git-send-email-pali.rohar@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
> 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.
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.
> > 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.
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.
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];
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(+)