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@xxxxxxxxxxxxxx>
> > ---
> > 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
> >
