linux-kernel - Re: [PATCH] of: address: Report error on resource bounds overflow

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: <CAL_JsqJ=7kX6DL_HBJMrWuhjZEmPUL++BvJ9tg3BDD9-e+b6Xw@mail.gmail.com>
Date: Thu, 5 Sep 2024 08:15:40 -0500
From: Rob Herring <robh@...nel.org>
To: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
Cc: Saravana Kannan <saravanak@...gle.com>, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Nam Cao <namcao@...utronix.de>
Subject: Re: [PATCH] of: address: Report error on resource bounds overflow

On Thu, Sep 5, 2024 at 2:46 AM Thomas Weißschuh
<thomas.weissschuh@...utronix.de> wrote:
>
> The members "start" and "end" of struct resource are of type
> "resource_size_t" which can be 32bit wide.
> Values read from OF however are always 64bit wide.
> Avoid silently truncating the value and instead return an error value.
>
> This can happen on real systems when the DT was created for a
> PAE-enabled kernel and a non-PAE kernel is actually running.
> For example with an arm defconfig and "qemu-system-arm -M virt".

A nice follow-up would be to make of_pci_range_to_resource() use
overflows_type() as well instead of open coding it.

> Link: https://bugs.launchpad.net/qemu/+bug/1790975
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
> Tested-by: Nam Cao <namcao@...utronix.de>
> Reviewed-by: Nam Cao <namcao@...utronix.de>
> ---
>  drivers/of/address.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index d669ce25b5f9..7e59283a4472 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -8,6 +8,7 @@
>  #include <linux/logic_pio.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
> +#include <linux/overflow.h>
>  #include <linux/pci.h>
>  #include <linux/pci_regs.h>
>  #include <linux/sizes.h>
> @@ -1061,7 +1062,11 @@ static int __of_address_to_resource(struct device_node *dev, int index, int bar_
>         if (of_mmio_is_nonposted(dev))
>                 flags |= IORESOURCE_MEM_NONPOSTED;
>
> +       if (overflows_type(taddr, r->start))
> +               return -EOVERFLOW;
>         r->start = taddr;

It looks odd that "r->start" is used before it is set, but I guess
overflows_type isn't using the value and the compiler would warn
otherwise.

Applied, thanks.

Rob