Re: [PATCH RFC 2/2] ACPI/IORT: work around num_ids ambiguity

[Robin Murphy <robin.murphy@arm.com>]

On 2020-05-01 2:10 pm, Ard Biesheuvel wrote:
> On Fri, 1 May 2020 at 14:31, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-05-01 12:41 pm, Ard Biesheuvel wrote:
>>> On Fri, 1 May 2020 at 12:55, Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> On 2020-05-01 10:58 am, Ard Biesheuvel wrote:
>>>>> The ID mapping table structure of the IORT table describes the size of
>>>>> a range using a num_ids field carrying the number of IDs in the region
>>>>> minus one. This has been misinterpreted in the past in the parsing code,
>>>>> and firmware is known to have shipped where this results in an ambiguity,
>>>>> where regions that should be adjacent have an overlap of one value.
>>>>>
>>>>> So let's work around this by detecting this case specifically: when
>>>>> resolving an ID translation, allow one that matches right at the end of
>>>>> a multi-ID region to be superseded by a subsequent one.
>>>>>
>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>>> ---
>>>>>     drivers/acpi/arm64/iort.c | 23 +++++++++++++++-----
>>>>>     1 file changed, 18 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>>> index 98be18266a73..d826dd9dc4c5 100644
>>>>> --- a/drivers/acpi/arm64/iort.c
>>>>> +++ b/drivers/acpi/arm64/iort.c
>>>>> @@ -316,10 +316,19 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
>>>>>         }
>>>>>
>>>>>         if (rid_in < map->input_base ||
>>>>> -         (rid_in >= map->input_base + map->id_count))
>>>>> +         (rid_in > map->input_base + map->id_count))
>>>>>                 return -ENXIO;
>>>>>
>>>>>         *rid_out = map->output_base + (rid_in - map->input_base);
>>>>> +
>>>>> +     /*
>>>>> +      * Due to confusion regarding the meaning of the id_count field (which
>>>>> +      * carries the number of IDs *minus 1*), we may have to disregard this
>>>>> +      * match if it is at the end of the range, and overlaps with the start
>>>>> +      * of another one.
>>>>> +      */
>>>>> +     if (map->id_count > 0 && rid_in == map->input_base + map->id_count)
>>>>> +             return -EAGAIN;
>>>>>         return 0;
>>>>>     }
>>>>>
>>>>> @@ -404,7 +413,8 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>>>>>         /* Parse the ID mapping tree to find specified node type */
>>>>>         while (node) {
>>>>>                 struct acpi_iort_id_mapping *map;
>>>>> -             int i, index;
>>>>> +             int i, index, rc = 0;
>>>>> +             u32 out_ref = 0, map_id = id;
>>>>>
>>>>>                 if (IORT_TYPE_MASK(node->type) & type_mask) {
>>>>>                         if (id_out)
>>>>> @@ -438,15 +448,18 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
>>>>>                         if (i == index)
>>>>>                                 continue;
>>>>>
>>>>> -                     if (!iort_id_map(map, node->type, id, &id))
>>>>> +                     rc = iort_id_map(map, node->type, map_id, &id);
>>>>> +                     if (!rc)
>>>>>                                 break;
>>>>
>>>> This needs a big FW_BUG splat in the case where it did find an overlap.
>>>
>>> Sure, although we did help create the problem in the first place.
>>>
>>>> Ideally we'd also enforce that the other half of must be the first entry
>>>> of another range, but perhaps we're into diminishing returns by that point.
>>>>
>>>
>>> That would mean the regions overlap regardless of whether you
>>> interpret num_ids correctly or not, which means we'll be doing
>>> validation of general well-formedness of the table rather than
>>> providing a workaround for this particular issue.
>>
>> The point was to limit any change in behaviour to the specific case that
>> we need to work around. Otherwise a table that was entirely malformed
>> rather than just off-by-one on the sizes might go from happening-to-work
>> to not working, or vice versa; the diminishing return is in how much we
>> care about that.
>>
> 
> I see. I think it is quite unlikely that a working system with
> overlapping ID ranges would work, and suddenly fail horribly when the
> exact point of overlap is shifted by 1. But yeah, I see your point.

Say that due to a copy-paste error or some other development artefact, 
the same correctly-sized input range is described twice, but the second 
copy has the wrong output base. Unless the IORT implementation is wacky 
enough to process mappings in reverse order it will have worked out OK, 
until suddenly the highest input ID starts falling through to the 
spurious broken mapping instead.

The match quirk implicitly encodes the exact nature of the ambiguity 
known to be present in the given table, so can be confident in fixing it 
up quietly. The heuristic doesn't have that luxury, so is wise to keep 
its scope as narrow as possible, and warn the user when it does choose 
to second-guess something on the off-chance that doing so actually makes 
the situation worse.

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel