linux-kernel - Re: [PATCH RFC 1/4] HID: treat fixed up report as const

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: <a48c4f98-bf7c-4050-807c-5c86fbbbd4f8@t-8ch.de>
Date: Sat, 3 Aug 2024 14:30:59 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Benjamin Tissoires <bentiss@...nel.org>
Cc: Jiri Kosina <jikos@...nel.org>, linux-input@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 1/4] HID: treat fixed up report as const

On 2024-07-31 15:59:21+0000, Benjamin Tissoires wrote:
> On Jul 30 2024, Thomas Weißschuh wrote:
> > Prepare the HID core for the ->report_fixup() callback to return const
> > data. This will then allow the HID drivers to store their static reports
> > in read-only memory.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > ---
> >  drivers/hid/hid-core.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 988d0acbdf04..dc233599ae56 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -1203,6 +1203,7 @@ int hid_open_report(struct hid_device *device)
> >  {
> >  	struct hid_parser *parser;
> >  	struct hid_item item;
> > +	const __u8 *fixed_up;
> >  	unsigned int size;
> >  	__u8 *start;
> >  	__u8 *buf;
> > @@ -1232,11 +1233,11 @@ int hid_open_report(struct hid_device *device)
> >  		return -ENOMEM;
> >  
> >  	if (device->driver->report_fixup)
> > -		start = device->driver->report_fixup(device, buf, &size);
> > +		fixed_up = device->driver->report_fixup(device, buf, &size);
> >  	else
> > -		start = buf;
> > +		fixed_up = buf;
> >  
> > -	start = kmemdup(start, size, GFP_KERNEL);
> > +	start = kmemdup(fixed_up, size, GFP_KERNEL);
> 
> I think that kmemdup makes all of your efforts pointless because from
> now, there is no guarantees that the report descriptor is a const.

The patch was meant to clarify the API to driver authors, not to make
the HID core safer. So I think it would not be pointless :-)

> How about you also change the struct hid_device to have both .dev_rdesc
> and .rdesc as const u8 *, and then also amend the function here so that
> start and end are properly handled?
> 
> This will make a slightly bigger patch but at least the compiler should
> then shout at us if we try to change the content of those buffers
> outside of the authorized entry points.

That sounds indeed like the correct solution.
It also completely avoids to introduction of yet another variable.

> Cheers,
> Benjamin
> 
> >  	kfree(buf);
> >  	if (start == NULL)
> >  		return -ENOMEM;
> > 
> > -- 
> > 2.45.2
> >