Hi,
On 8/25/22 19:07, Andy Shevchenko wrote:
> On Thu, Aug 25, 2022 at 8:04 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> 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.
>>
>> 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:
>
> Yep, that's what I had in my mind, but was in doubt about the case in
> general. Hence the discussion. Yet, w/o this patch prevents us to call
> the mentioned match functions when there is no guarantee that match is
> there. That said, you may add my
>
> Reported-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>
> to the below patch when formally sent.
Replying to make sure this does not get accidentally applied:
NACK. See my other email in this thread.
Regards,
Hans
>
>> ---
>> 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;
>
>
>
