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: <201601121849.44256@pali>
Date:	Tue, 12 Jan 2016 18:49:44 +0100
From:	Pali Rohár <pali.rohar@...il.com>
To:	Michał Kępień <kernel@...pniu.pl>
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

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@...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;

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@...il.com

Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)