linux-kernel - Re: [RFC PATCH 0/2] Handling of non-numbered feature reports by hidraw

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:   Thu, 8 Dec 2022 12:58:53 -0800
From:   Andrey Smirnov <andrew.smirnov@...il.com>
To:     David Rheinsberg <david.rheinsberg@...il.com>
Cc:     linux-input@...r.kernel.org, Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC PATCH 0/2] Handling of non-numbered feature reports by hidraw

On Thu, Dec 8, 2022 at 7:46 AM David Rheinsberg
<david.rheinsberg@...il.com> wrote:
>
> Hi
>
> On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@...il.com> wrote:
> > I'm working on a firmware of a device that exposes a HID interface via
> > USB and/or BLE and uses, among other things, non-numbered feature
> > reports. Included in this series are two paches I had to create in
> > order for hidraw devices created for aforementioned subsystems to
> > behave in the same way when exerciesd by the same test tool.
> >
> > I don't know if the patches are acceptable as-is WRT to not breaking
> > existing userspace, hence the RFC tag.
>
> Can you elaborate why you remove the special handling from USBHID but
> add it to UHID? They both operate logically on the same level, so
> shouldn't we simply adjust uhid to include the report-id in buf[0]?
>
> Also, you override buf[0] in UHID, so I wonder what UHID currently
> returns there?
>
> IOW, can you elaborate a bit what the current behavior of each of the
> involved modules is, and what behavior you would expect? This would
> allow to better understand what you are trying to achieve. The more
> context you can give, the easier it is to understand what happens
> there.
>

Sorry it's not very clear, so the difference between the cases is that
in the case of UHID the report ID ends up being included as a part of
"SET_FEATURE", so BlueZ checks UHID_DEV_NUMBERED_FEATURE_REPORTS,
which is not set (correctly) and tries to send the whole payload. This
ends up as a maxlen + 1 (extra byte) write to a property that is
maxlen long, which gets rejected by device's BLE stack.

In the case of USBHID the problem happens in "GET_FEATURE" path. When
userspace reads the expected data back it gets an extra 0 prepended to
the payload, so all of the actual payload has an offset of 1. This
doesn't happen with UHID, which I think is the correct behavior here.

Hopefully that explains the difference, let me know if something is unclear