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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160112111439.GA7630@eudyptula.hq.kempniu.pl>
Date:	Tue, 12 Jan 2016 12:14:39 +0100
From:	Michał Kępień <kernel@...pniu.pl>
To:	Pali Rohár <pali.rohar@...il.com>
Cc:	Matthew Garrett <mjg59@...f.ucam.org>,
	Darren Hart <dvhart@...radead.org>,
	Gabriele Mazzotta <gabriele.mzt@...il.com>,
	Andy Lutomirski <luto@...nel.org>,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: 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@...il.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ń