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

[Ard Biesheuvel <ardb@kernel.org>]

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.

I think the fact that we got it wrong initially justifies treating the
off-by-one case specially, but beyond that, we should make it robust
without being pedantic imo.

> If we silently fix things up, then people will continue to write broken
> tables without even realising, new OSes will have to implement the same
> mechanism because vendors will have little interest in changing things
> that have worked "correctly" with Linux for years, and we've effectively
> achieved a de-facto redefinition of the spec. Making our end of the
> interface robust is obviously desirable, but there still needs to be
> *some* incentive for the folks on the other end to get it right.
>

Agreed. But at least we'll be able to detect it and flag it in the
general case, rather than having a special case for D05/06 only
(although I suppose splitting the output ranges like those platforms
do is rather unusual)