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.
--
With Best Regards,
Andy Shevchenko
