diff mbox series

[RFC,1/4] HID: treat fixed up report as const

Message ID 20240730-hid-const-fixup-v1-1-f667f9a653ba@weissschuh.net
State New
Headers show
Series [RFC,1/4] HID: treat fixed up report as const | expand

Commit Message

Thomas Weißschuh July 30, 2024, 9:35 p.m. UTC
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@weissschuh.net>
---
 drivers/hid/hid-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Thomas Weißschuh Aug. 3, 2024, 12:30 p.m. UTC | #1
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@weissschuh.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
> >
diff mbox series

Patch

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);
 	kfree(buf);
 	if (start == NULL)
 		return -ENOMEM;