linux-kernel - Re: [PATCH v5 2/2] Input: add Apple SPI keyboard and trackpad driver.

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] [day] [month] [year] [list]

Date:   Tue, 16 Apr 2019 15:52:34 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     "Life is hard, and then you die" <ronald@...ovation.ch>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Henrik Rydberg <rydberg@...math.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Inki Dae <inki.dae@...sung.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Lukas Wunner <lukas@...ner.de>,
        Federico Lorenzi <federico@...velground.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        linux-input@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/2] Input: add Apple SPI keyboard and trackpad driver.

On Mon, Apr 15, 2019 at 04:09:55PM -0700, Life is hard, and then you die wrote:
> On Mon, Apr 15, 2019 at 12:03:46PM +0300, Andy Shevchenko wrote:
> > On Mon, Apr 15, 2019 at 01:13:00AM -0700, Ronald Tschalär wrote:

> > > +static void
> > > +applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
> > > +{
> > > +	unsigned char tmp;
> > 
> > > +	u8 bit = BIT(fnremap - 1);
> > 
> > The above is UB and I'm sorry I didn't find this earlier.
> > 
> > So, something like this would work
> > 
> > 	u8 bit = BIT((fnremap - 1) & 0x07);
> 
> fnremap is already constrained by the following:

"already" is a wrong word here. Compiler checks in the order of appearing, so,
for it it is UB and we need to limit bits to allowed range, up to 31 for
unsigned int.

> 
> > > +
> > > +	if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
> > > +	    !applespi_controlcodes[fnremap - 1])
> > > +		return;
> 
> and the array-size of applespi_controlcodes is constrained to the
> number of bits in u8 according to this assertion
> 
> > > +	compiletime_assert(ARRAY_SIZE(applespi_controlcodes) ==
> > > +			   sizeof_field(struct keyboard_protocol, modifiers) * 8,
> > > +			   "applespi_controlcodes has wrong number of entries");
> 
> So I don't see that the masking buys anything new.

It buys us the follow to the standard. But gcc is clever enough to strip number
to allowed one.

-- 
With Best Regards,
Andy Shevchenko