| Message ID | 20240730-hid-const-fixup-v1-1-f667f9a653ba@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: constify static fixed up report descriptors | expand |
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. 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 >
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(-)