On Fri, May 01, 2020 at 06:10:14PM +0200, 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.
>
> To prevent potential regressions on broken firmware that happened to
> work before, only take the subsequent match into account if it occurs
> at the start of a mapping region.
>
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
> drivers/acpi/arm64/iort.c | 40 +++++++++++++++++---
> 1 file changed, 34 insertions(+), 6 deletions(-)
The patch logic is sound - I still think that the resulting code can
benefit from a one-off boot time mapping data initialisation but we can
address that later as a clean-up, first thing is to remove the quirk
mechanism.
Goes without saying, this needs extensive testing on existing
platforms before sending it to stable kernels.
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 98be18266a73..9f139a94a1d3 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -300,7 +300,7 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
> }
>
> static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
> - u32 *rid_out)
> + u32 *rid_out, bool check_overlap)
> {
> /* Single mapping does not care for input id */
> if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
> @@ -316,10 +316,34 @@ 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;
>
> + if (check_overlap) {
> + /*
> + * We already found a mapping for this input ID at the end of
> + * another region. If it coincides with the start of this
> + * region, we assume the prior match was due to the off-by-1
> + * issue mentioned below, and allow it to be superseded.
> + * Otherwise, things are *really* broken, and we just disregard
> + * duplicate matches entirely to retain compatibility.
> + */
> + pr_err(FW_BUG "[map %p] conflicting mapping for input ID 0x%x\n",
> + map, rid_in);
> + if (rid_in != map->input_base)
> + 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 +428,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 +463,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, out_ref);
> + if (!rc)
> break;
> + if (rc == -EAGAIN)
> + out_ref = map->output_reference;
> }
>
> - if (i == node->mapping_count)
> + if (i == node->mapping_count && !out_ref)
> goto fail_map;
>
> node = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
> - map->output_reference);
> + rc ? out_ref : map->output_reference);
> }
>
> fail_map:
> --
> 2.17.1
>
