linux-kernel - Re: [PATCH 1/2] Input; Sanitize event code before modifying bitmaps

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]

Message-ID: <bcf65dce766d0376027e80bf205b095d@kernel.org>
Date:   Wed, 26 Aug 2020 14:32:38 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH 1/2] Input; Sanitize event code before modifying bitmaps

Hi Dmitry,

On 2020-08-24 20:51, Dmitry Torokhov wrote:
> Hi Marc,
> 
> On Mon, Aug 17, 2020 at 12:26:59PM +0100, Marc Zyngier wrote:
>> When calling into input_set_capability(), the passed event code is
>> blindly used to set a bit in a number of bitmaps, without checking
>> whether this actually fits the expected size of the bitmap.
>> 
>> This event code can come from a variety of sources, including devices
>> masquerading as input devices, only a bit more "programmable".
>> 
>> Instead of taking the raw event code, sanitize it to the actual bitmap
>> size and output a warning to let the user know.
>> 
>> These checks are, at least in spirit, in keeping with cb222aed03d7
>> ("Input: add safety guards to input_set_keycode()").
>> 
>> Cc: stable@...r.kernel.org
>> Signed-off-by: Marc Zyngier <maz@...nel.org>
>> ---
>>  drivers/input/input.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> index 3cfd2c18eebd..1e77cf47aa44 100644
>> --- a/drivers/input/input.c
>> +++ b/drivers/input/input.c
>> @@ -1974,14 +1974,18 @@ EXPORT_SYMBOL(input_get_timestamp);
>>   * In addition to setting up corresponding bit in appropriate 
>> capability
>>   * bitmap the function also adjusts dev->evbit.
>>   */
>> -void input_set_capability(struct input_dev *dev, unsigned int type, 
>> unsigned int code)
>> +void input_set_capability(struct input_dev *dev, unsigned int type, 
>> unsigned int raw_code)
>>  {
>> +	unsigned int code = raw_code;
>> +
>>  	switch (type) {
>>  	case EV_KEY:
>> +		code &= KEY_MAX;
>>  		__set_bit(code, dev->keybit);
> 
> 
> I would much rather prefer we did not simply set some random bits in
> this case, but instead complained loudly and refused to alter anything.
> 
> The function is not exported directly to userspace, so we expect 
> callers
> to give us sane inputs, and I believe WARN() splash in case of bad
> inputs would be the best solution here.

Fair enough. I've moved the checking to the HID layer (using
hid_map_usage() as the validation primitive), which makes
changing input_set_capability() irrelevant.

I'll post v2 shortly in the form of a single patch.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...