Re: [PATCH 00/11] HID: storing pointers in 'hid_device_id::driver_data'

[Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>]

> I would agree with sashiko that the series introduces an anti-pattern by
> allowing .driver_data to be an arbitrary pointer. The reason is that we
> can dynamically set driver_data through the kernel command line, and
> when it's used as a pointer, bad things will happen.

I have not considered the command line override ! This indeed would not
be a good case for having a pointer at all in there - in the current
form of the codebase.

The series does not introduce this anti-pattern - this is already the
current status quo in the codebase and the problem predates the patch
series itself. The series makes the problem more visible and validates
the existing status quo - agreed on this.

> I would think we should fix the 2 offenders to enforce not using a
> direct pointer but an offset in a table of pointers.
>
> How does that sound for you?

That could work, probably via a named enum (so that the idx is bounded
and validated to avoid out of bounds memory access) with its items mapped
into a table of const pointers. That way the command line override still
works as before and my goals set out in the series are met as well.

Perhaps an alternative solution would be to sanitize the user-input instead in
the 'hid-core::new_id_store' function, such that only 'driver_data' values
that match an existing entry in the driver's 'id_table' are accepted, this
is how it is done currently in the PCI subsystem in 'pci-driver::new_id_store',
so it would be consistent as well.

The bonus here is that all other drivers in the  HID subsystem benefit from
this aproach as opposed to just the two current offenders, as providing
'driver_data' from the command line is now mandatory and must match the
existing table entry - which is what we want ?

So something among the lines of:

@hid-core::new_id_store

```
struct hid_driver *hdrv = to_hid_driver(drv);
const struct hid_device_id *ids = hdrv->id_table;

(...)

/* Only accept driver_data values that match an existing id_table
    entry */
if (ids) {
        ret = -EINVAL;
        while (ids->vendor || ids->product) {
                if (driver_data == ids->driver_data) {
                        ret = 0;
                        break;
                }
                ids++;
        }
        if (ret)        /* No match */
                return ret;
}
```

Would that approach work for you ? Do help in what is the correct
termination for the while loop - for this demo I went with 'ids->vendor'
and 'ids->product' fields as common-sense would indicate that quirks need
to be device-specific by definition but that was without any actual research.

The downside is that 'new_id' drivers that actually use a pointer for the
'driver_data' will be rejected - which would be actually covered by the
solution that you have proposed in the first place. To think of it the
input sanitizer could work additionaly in tandem with the enum-based
solution if this fact is an issue, so it might not be an alternative
actually but a complement for it.

In the meantime I think that patches 1-8 can be reviewed regardless
without any changes (as they predate patch 9 that adds the union and the
commit messages does not mention anything about it).

Best regards,
Pawel