On Thu, Aug 25, 2022 at 7:09 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 8/25/22 19:03, Rafael J. Wysocki wrote:
> > On Thu, Aug 25, 2022 at 6:48 PM Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx> wrote:
> >>
> >> On Thu, Aug 25, 2022 at 3:48 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> >>> On Thu, Aug 25, 2022 at 2:38 PM Andy Shevchenko
> >>> <andy.shevchenko@xxxxxxxxx> wrote:
> >>>>
> >>>> I have stumbled over __acpi_match_device() implementation and noticed
> >>>> different types of termination of the struct acpi_device_id (ACPI ID
> >>>> list), i.e. '{ }' vs. '{"", 0}'.
> >>>>
> >>>> As I read the code of the above mentioned function, I see that it
> >>>> dereferences the id field without NULL check. This means we are quite
> >>>> lucky (somebody before guarantees the match) we have no crash here.
> >>>
> >>> I'm not sure what you mean.
> >>>
> >>> In __acpi_match_device() id is a pointer used for walking the acpi_ids
> >>> table (if not NULL). Its initial value is the acpi_ids value and it's
> >>> incremented in every step, so it cannot be NULL.
> >>>
> >>> The loop is terminated if both the first byte of the device ID field
> >>
> >> ^^^ (1)
> >>
> >>> and the cls field in the current row are both zeros, so both
> >>> termination markers in use should work.
> >>>
> >>> Or am I missing anything?
> >>
> >> Yes. The ID field itself is _dereferenced_ w/o NULL check. So, compare
> >> two ID lists:
> >>
> >> FIRST:
> >> { "A", 1 },
> >> { "B", 2 },
> >> { "", 0}
> >>
> >> SECOND:
> >> { "A", 1 },
> >> { "B", 2 },
> >> { }
> >>
> >> They are different in the terminator and the above mentioned function
> >> simply will crash the kernel if no match is found. Of course I might
> >> miss something, but as I said it seems we are simply lucky that
> >> somebody else (platform / device core code?) does our job.
>
> No they are not different, the id field is not a "char *" as
> I believe you are thinking. The id field actually is a pre-allocated
> array of length ACPI_ID_LEN:
>
> struct acpi_device_id {
> __u8 id[ACPI_ID_LEN];
> kernel_ulong_t driver_data;
> __u32 cls;
> __u32 cls_msk;
> };
>
> So in both terminators above id[] will be set to 0 and there is
> no problem other then the style being inconsistent.
>
>
> >
> > OK, I see. id->id[0] doesn't work if id->id is NULL which it is in
> > the second case.
> >
> > I think it doesn't crash in practice, because it's always called when
> > there's a match.
> >
> > Anyway, something like this would fix it, wouldn't it:
> >
> > ---
> > drivers/acpi/bus.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/bus.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/bus.c
> > +++ linux-pm/drivers/acpi/bus.c
> > @@ -868,8 +868,8 @@ static bool __acpi_match_device(struct a
> > list_for_each_entry(hwid, &device->pnp.ids, list) {
> > /* First, check the ACPI/PNP IDs provided by the caller. */
> > if (acpi_ids) {
> > - for (id = acpi_ids; id->id[0] || id->cls; id++) {
> > - if (id->id[0] && !strcmp((char *)id->id, hwid->id))
> > + for (id = acpi_ids; (id->id && id->id[0]) || id->cls; id++) {
> > + if (id->id && id->id[0] && !strcmp((char *)id->id, hwid->id))
> > goto out_acpi_match;
> > if (id->cls && __acpi_match_device_cls(id, hwid))
> > goto out_acpi_match;
> >
>
> This change is not necessary, see above.
You're right and I forgot about it.
