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

[MichaÅ KÄpieÅ]

> 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@xxxxxxxxx>
> ---
>  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Å