| 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 |
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 --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;
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(-)