linux-kernel - Re: [PATCH 2/3] HID: steam: add serial number information.

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: <20180216205939.GA17329@casa>
Date:   Fri, 16 Feb 2018 21:59:39 +0100
From:   Rodrigo Rivas Costa <rodrigorivascosta@...il.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>, linux-input@...r.kernel.org
Subject: Re: [PATCH 2/3] HID: steam: add serial number information.

On Fri, Feb 16, 2018 at 11:38:11AM +0100, Benjamin Tissoires wrote:
> On Fri, Feb 16, 2018 at 10:57 AM, Rodrigo Rivas Costa
> <rodrigorivascosta@...il.com> wrote:
> > On Fri, Feb 16, 2018 at 10:31:35AM +0100, Benjamin Tissoires wrote:
> >> > Ok, I'll do that. The weird thing, however, is that:
> >> >
> >> >         hid_hw_raw_request(steam->hid_dev, 0x00,
> >> >                 buf, hid_report_len(r), /* 64 */
> >> >                 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> >> >
> >> > fails with EOVERFLOW. I have to use:
> >> >
> >> >         hid_hw_raw_request(steam->hid_dev, 0x00,
> >> >                 buf, 65
> >> >                 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> >> >
> >> > which just feels wrong to me.
> >>
> >> Indeed
> >>
> 
> Arf, looks like usbhid expects the buffer to start with 0x00 when the
> report is not numbered, thus adding one to the report length.
> 
> I guess that nobody tried to recently set/get reports on a device
> without a report ID. And hidraw matches this behavior too, which means
> it's hard to change.
> 
> One thing I'd like to try to change is the result of hid_report_len.
> If everybody expects the size to be of one more when the report is
> unnumbered, maybe we could simply add this placeholder in the report
> size from the beginning.

I've been trying to make sense of all this numbered/buf++/count-- stuff,
and I admit I don't understand half of it...

But about that +7 in hid_alloc_report_buf(), isn't it to make room for
the implement()/extract() functions? And IIUIC those are not used for
raw_requests... they are instead passed directly to usb_control_msg()
(or whatever the ll driver does). That's the point of being raw, isn't
it?

If I'm right with that, would it make sense to go back to kzalloc(65)?

If I'm wrong, then if you agree, I'll default to:

	hid_hw_raw_request(steam->hid_dev, 0x00,
		buf, hid_report_len(r) + 1, /* count the request number	*/
		HID_FEATURE_REPORT, HID_REQ_GET_REPORT);

Then, if hid_report_len() is ever updated to return the +1, this one
should be removed. And even if it is not, we still have +6 extra bytes
from hid_alloc_report_buf(), so no real harm done.

Regards.
Rodrigo