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.
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;
