linux-kernel - Re: [PATCH] of: property: Fix fw_devlink handling of interrupt-map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGETcx-ZbK3P+CoPb1szjWNBbM7kN0j_p6QzT4iqPrYHqvWjRw@mail.gmail.com>
Date: Tue, 28 May 2024 19:23:39 +0200
From: Saravana Kannan <saravanak@...gle.com>
To: Marc Zyngier <maz@...nel.org>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-riscv@...ts.infradead.org, 
	Anup Patel <apatel@...tanamicro.com>, Rob Herring <robh@...nel.org>
Subject: Re: [PATCH] of: property: Fix fw_devlink handling of interrupt-map

On Tue, May 28, 2024 at 6:41 PM Marc Zyngier <maz@...nel.org> wrote:
>
> Commit d976c6f4b32c ("of: property: Add fw_devlink support for
> interrupt-map property") tried to do what it says on the tin,
> but failed on a couple of points:
>
> - it confuses bytes and cells. Not a huge deal, except when it
>   comes to pointer arithmetic
>
> - it doesn't really handle anything but interrupt-maps that have
>   their parent #address-cells set to 0
>
> The combinations of the two leads to some serious fun on my M1
> box, with plenty of WARN-ON() firing all over the shop, and
> amusing values being generated for interrupt specifiers.
>
> Address both issues so that I can boot my machines again.
>
> Fixes: d976c6f4b32c ("of: property: Add fw_devlink support for interrupt-map property")
> Signed-off-by: Marc Zyngier <maz@...nel.org>
> Cc: Anup Patel <apatel@...tanamicro.com>
> Cc: Saravana Kannan <saravanak@...gle.com>
> Cc: Rob Herring (Arm) <robh@...nel.org>
> ---
>  drivers/of/property.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 1c83e68f805b..9adebc63bea9 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1322,7 +1322,13 @@ static struct device_node *parse_interrupt_map(struct device_node *np,
>         addrcells = of_bus_n_addr_cells(np);
>
>         imap = of_get_property(np, "interrupt-map", &imaplen);
> -       if (!imap || imaplen <= (addrcells + intcells))
> +       imaplen /= sizeof(*imap);
> +
> +       /*
> +        * Check that we have enough runway for the child unit interrupt
> +        * specifier and a phandle. That's the bare minimum we can expect.
> +        */
> +       if (!imap || imaplen <= (addrcells + intcells + 1))
>                 return NULL;
>         imap_end = imap + imaplen;
>
> @@ -1346,8 +1352,14 @@ static struct device_node *parse_interrupt_map(struct device_node *np,
>                 if (!index)
>                         return sup_args.np;
>
> -               of_node_put(sup_args.np);
> +               /*
> +                * Account for the full parent unit interrupt specifier
> +                * (address cells, interrupt cells, and phandle).
> +                */
> +               imap += of_bus_n_addr_cells(sup_args.np);
>                 imap += sup_args.args_count + 1;
> +
> +               of_node_put(sup_args.np);
>                 index--;
>         }
>

Thanks Marc! And sorry for not catching this in my earlier review!

Acked-by: Saravana Kannan <saravanak@...gle.com>

-Saravana