linux-kernel - Re: [PATCH 4.19 146/191] HID: wacom: Avoid entering wacom_wac_pen

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]

Date:   Wed, 4 Nov 2020 20:06:27 +0100
From:   Pavel Machek <pavel@....cz>
To:     "Gerecke, Jason" <Jason.Gerecke@...om.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jason Gerecke <killertofu@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "Cheng, Ping" <Ping.Cheng@...om.com>, Jiri Kosina <jkosina@...e.cz>
Subject: Re: [PATCH 4.19 146/191] HID: wacom: Avoid entering
 wacom_wac_pen_report for pad / battery

Hi!

> > > To correct this, we restore a version of the `WACOM_PAD_FIELD` check
> > > in `wacom_wac_collection()` and return early. This effectively prevents
> > > pad / battery collections from being reported until the very end of the
> > > report as originally intended.
> > 
> > Okay... but code is either wrong or very confusing:
> > 
> > > +++ b/drivers/hid/wacom_wac.c
> > > @@ -2729,7 +2729,9 @@ static int wacom_wac_collection(struct h
> > >        if (report->type != HID_INPUT_REPORT)
> > >                return -1;
> > >  
> > > -     if (WACOM_PEN_FIELD(field) && wacom->wacom_wac.pen_input)
> > > +     if (WACOM_PAD_FIELD(field))
> > > +             return 0;
> > > +     else if (WACOM_PEN_FIELD(field) && wacom->wacom_wac.pen_input)
> > >                wacom_wac_pen_report(hdev, report);
> > 
> > wacom_wac_pen_report() can never be called, because WACOM_PEN_FIELD()
> > can not be true here; if it was we'd return in the line above.
> 
> For reference, here's the definition for WACOM_PAD_FIELD() and WACOM_PEN_FIELD():
> 
> > #define WACOM_PAD_FIELD(f)	(((f)->physical == HID_DG_TABLETFUNCTIONKEY) || \
> > 				 ((f)->physical == WACOM_HID_WD_DIGITIZERFNKEYS) || \
> > 				 ((f)->physical == WACOM_HID_WD_DIGITIZERINFO))
> > 
> > #define WACOM_PEN_FIELD(f)	(((f)->logical == HID_DG_STYLUS) || \
> > 				 ((f)->physical == HID_DG_STYLUS) || \
> > 				 ((f)->physical == HID_DG_PEN) || \
> > 				 ((f)->application == HID_DG_PEN) || \
> > 				 ((f)->application == HID_DG_DIGITIZER) || \
> > 				 ((f)->application == WACOM_HID_WD_PEN) || \
> > 				 ((f)->application == WACOM_HID_WD_DIGITIZER) || \
> > 				 ((f)->application == WACOM_HID_G9_PEN) || \
> > 				 ((f)->application == WACOM_HID_G11_PEN))
> 
> WACOM_PAD_FIELD() evaluates to `true` for pad data *not* pen data because pen data is not inside any of the 3 physical collections its looks for.
> 
> WACOM_PEN_FIELD() evaluates to `true` for pad data *and* pen data because both types of data are inside of the Digitizer application collection.
>

> Without the WACOM_PAD_FIELD() check in place at the very beginning, both pad and pen data would trigger a call to wacom_wac_pen_report(). This is undesired: only pen data should result in that function being called. Adding the check causes the function to return early for pad data while pen data falls into the "else if" and is processed as before. Pad data is only reported once the entire report has been valuated by making a call to wacom_wac_pad_report() at the very end of wacom_wac_report().
>

Yep, you are right. I failed to notice the difference between _PAD_
and _PEN_ macros, and so the code did not make sense.

Sorry for the noise.

Best regards,
								Pavel
								
-- 
http://www.livejournal.com/~pavelmachek

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