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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2kfmgwlmliwmn6olmnaab2mdn4ywquqputk3hcdqqkyqc6bfvd@jtlmixoar7qu>
Date: Wed, 31 Jul 2024 15:59:21 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Thomas Weißschuh <linux@...ssschuh.net>
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 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.

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.

Cheers,
Benjamin

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