linux-hardening - Re: [PATCH] HID: usbhid: fix recurrent out-of-bounds bug in usbhid

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: <649f5c1c-f3a7-420d-b727-f904d58f491f@rowland.harvard.edu>
Date: Tue, 28 Jan 2025 12:00:41 -0500
From: Alan Stern <stern@...land.harvard.edu>
To: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
Cc: Kees Cook <kees@...nel.org>, Jiri Kosina <jikos@...nel.org>,
	Benjamin Tissoires <bentiss@...nel.org>, linux-usb@...r.kernel.org,
	linux-input@...r.kernel.org, syzkaller-bugs@...glegroups.com,
	linux-kernel@...r.kernel.org,
	syzbot+c52569baf0c843f35495@...kaller.appspotmail.com,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH] HID: usbhid: fix recurrent out-of-bounds bug in
 usbhid_parse()

On Tue, Jan 28, 2025 at 05:45:21AM -0800, Nikita Zhandarovich wrote:
> Hello,
> 
> On 6/4/24 10:45, Alan Stern wrote:
> > On Tue, Jun 04, 2024 at 10:21:15AM -0700, Kees Cook wrote:
> >> On Tue, Jun 04, 2024 at 10:09:43AM -0700, Nikita Zhandarovich wrote:
> >>> Hi,
> >>>
> >>> On 6/4/24 07:15, Jiri Kosina wrote:
> >>>> On Tue, 4 Jun 2024, Kees Cook wrote:
> >>>>
> >>>>> This isn't the right solution. The problem is that hid_class_descriptor 
> >>>>> is a flexible array but was sized as a single element fake flexible 
> >>>>> array:
> >>>>>
> >>>>> struct hid_descriptor {
> >>>>> 	   __u8  bLength;
> >>>>> 	   __u8  bDescriptorType;
> >>>>> 	   __le16 bcdHID;
> >>>>> 	   __u8  bCountryCode;
> >>>>> 	   __u8  bNumDescriptors;
> >>>>>
> >>>>> 	   struct hid_class_descriptor desc[1];
> >>>>> } __attribute__ ((packed));
> >>>>>
> >>>>> This likely needs to be: 
> >>>>>
> >>>>> struct hid_class_descriptor desc[] __counted_by(bNumDescriptors);
> >>>>>
> >>>>> And then check for any sizeof() uses of the struct that might have changed.
> 
> Alan, I finally got around to preparing a revised version of the
> required patch and encountered a few issues. I could use some advice in
> this matter...
> 
> If we change 'struct hid_descriptor' as you suggested,

I didn't make that suggestion.  Kees Cook did.

>  which does make
> sense, most occurrences of that type are easy enough to fix.
> 
> 1) usbhid_parse() starts working properly if there are more than 1
> descriptors, sizeof(struct hid_descriptor) may be turned into something
> crude but straightforward like sizeof(struct hid_descriptor) +
> sizeof(struct hid_class_descriptor).
> 
> 2) 'hid_descriptor' in drivers/hid/hid-hyperv.c remains innocuous as
> well as only 1 descriptor expected there. My impression is only some
> small changes are needed there.
> 
> However, the issue that stumps me is the following: static struct
> hid_descriptor hidg_desc in drivers/usb/gadget/function/f_hid.c relies
> on a static nature of that one descriptor. hidg_desc ends up being used
> elsewhere, in other static structures. Basically, using __counted_by
> requires a lot of changes, as I see it, out of scope of merely closing
> an UBSAN error.

The hidg_desc structure needs to contain room for a single 
hid_descriptor containing a single hid_class_descriptor.  I think you 
can define it that way by doing something like this:

static struct hid_descriptor hidg_desc = {
	.bLength			= sizeof hidg_desc,
	.bDescriptorType		= HID_DT_HID,
	.bcdHID				= cpu_to_le16(0x0101),
	.bCountryCode			= 0x00,
	.bNumDescriptors		= 0x1,
	.desc				= {
		{
			.bDescriptorType	= 0, /* DYNAMIC */
			.wDescriptorLength	= 0, /* DYNAMIC */
		}
	}
};

Or maybe it needs to be:

	.desc				= { {0, 0} } /* DYNAMIC */

I'm not sure what is the correct syntax; you'll have to figure that out.  

You'll have to be more careful about the definition of hidg_desc_copy in 
hidg_setup(), however.  You might want to define hidg_desc_copy as an 
alias to the start of a byte array of the right size.

> Is this approach still worthy pursuing or should I look into some neater
> solution?

I think you should persist with this approach.

Alan Stern