Message-ID: <CAL_JsqLciDTxfeKwuNNWEOZjrUDFp9g7ZAzTuY4nQ1GCwPmaow@mail.gmail.com>
Date: Wed, 7 Aug 2024 08:07:07 -0600
From: Rob Herring <robh@...nel.org>
To: Stefan Wiehler <stefan.wiehler@...ia.com>
Cc: Saravana Kannan <saravanak@...gle.com>, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] of/irq: Consider device address size in interrupt map walk

On Wed, Aug 7, 2024 at 6:04 AM Stefan Wiehler <stefan.wiehler@...ia.com> wrote:
>
> When of_irq_parse_raw() is invoked with a device address smaller than
> the interrupt parent node (from #address-cells property), KASAN detects
> the following out-of-bounds read when populating the initial match table
> (dyndbg="func of_irq_parse_* +p"):

You've missed a bunch of people/lists. Use get_maintainers.pl.

>   OF: of_irq_parse_one: dev=/soc@...icasso/watchdog, index=0
>   OF:  parent=/soc@...ci@...000000000/gpio0@17,0, intsize=2
>   OF:  intspec=4
>   OF: of_irq_parse_raw: ipar=/soc@...ci@...000000000/gpio0@17,0, size=2
>   OF:  -> addrsize=3

Can you provide some details on what these nodes look like. The
interrupt provider to an SoC device is a PCI device? That's weird...


>   ==================================================================
>   BUG: KASAN: slab-out-of-bounds in of_irq_parse_raw+0x2b8/0x8d0
>   Read of size 4 at addr ffffff81beca5608 by task bash/764
>
>   CPU: 1 PID: 764 Comm: bash Tainted: G           O       6.1.67-484c613561-nokia_sm_arm64 #1

Note that of_irq_parse_raw() was refactored in 6.10, so your patch is
not going to apply.

>   Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2023.01-12.24.03-dirty 01/01/2023
>   Call trace:
>    dump_backtrace+0xdc/0x130
>    show_stack+0x1c/0x30
>    dump_stack_lvl+0x6c/0x84
>    print_report+0x150/0x448
>    kasan_report+0x98/0x140
>    __asan_load4+0x78/0xa0
>    of_irq_parse_raw+0x2b8/0x8d0
>    of_irq_parse_one+0x24c/0x270
>    parse_interrupts+0xc0/0x120
>    of_fwnode_add_links+0x100/0x2d0
>    fw_devlink_parse_fwtree+0x64/0xc0
>    device_add+0xb38/0xc30
>    of_device_add+0x64/0x90
>    of_platform_device_create_pdata+0xd0/0x170
>    of_platform_bus_create+0x244/0x600
>    of_platform_notify+0x1b0/0x254
>    blocking_notifier_call_chain+0x9c/0xd0
>    __of_changeset_entry_notify+0x1b8/0x230
>    __of_changeset_apply_notify+0x54/0xe4
>    of_overlay_fdt_apply+0xc04/0xd94

I wonder if it is possible for KASAN to detect this if it is a base DT
given the allocation is done by memblock.

>    ...
>
>   The buggy address belongs to the object at ffffff81beca5600
>    which belongs to the cache kmalloc-128 of size 128
>   The buggy address is located 8 bytes inside of
>    128-byte region [ffffff81beca5600, ffffff81beca5680)
>
>   The buggy address belongs to the physical page:
>   page:00000000230d3d03 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1beca4
>   head:00000000230d3d03 order:1 compound_mapcount:0 compound_pincount:0
>   flags: 0x8000000000010200(slab|head|zone=2)
>   raw: 8000000000010200 0000000000000000 dead000000000122 ffffff810000c300
>   raw: 0000000000000000 0000000000200020 00000001ffffffff 0000000000000000
>   page dumped because: kasan: bad access detected
>
>   Memory state around the buggy address:
>    ffffff81beca5500: 04 fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>    ffffff81beca5580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>   >ffffff81beca5600: 00 fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>                         ^
>    ffffff81beca5680: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>    ffffff81beca5700: 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc
>   ==================================================================
>   OF:  -> got it !
>
> Signed-off-by: Stefan Wiehler <stefan.wiehler@...ia.com>
> ---
>  arch/powerpc/platforms/fsl_uli1575.c |  2 +-
>  drivers/bcma/main.c                  |  2 +-
>  drivers/of/irq.c                     | 64 ++++++++++++++++------------
>  drivers/pci/of.c                     |  2 +-
>  include/linux/of_irq.h               |  3 +-
>  5 files changed, 41 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/platforms/fsl_uli1575.c b/arch/powerpc/platforms/fsl_uli1575.c
> index b8d37a9932f1b..8da36f72b7b48 100644
> --- a/arch/powerpc/platforms/fsl_uli1575.c
> +++ b/arch/powerpc/platforms/fsl_uli1575.c
> @@ -335,7 +335,7 @@ static void hpcd_final_uli5288(struct pci_dev *dev)
>         oirq.args_count = 1;
>         laddr[0] = (hose->first_busno << 16) | (PCI_DEVFN(31, 0) << 8);
>         laddr[1] = laddr[2] = 0;
> -       of_irq_parse_raw(laddr, &oirq);
> +       of_irq_parse_raw(laddr, ARRAY_SIZE(laddr), &oirq);

That's not the right information to parse the address correctly. You
would need the device's #address-cells. However, in most cases we
don't really need to parse the address. The address is not really used
except in cases where interrupt routing matches the bus and so there
is only one size. That's effectively only PCI devices today. In that
case, the address size would always be equal, and the code implicitly
assumes that. It would be better if we could just detect when to use
the address or not. I think we'd have to look at 'interrupt-map-mask'
first to see whether or not to read the address cells. Or maybe we
could detect when the interrupt parent is the device's parent node.
Either way, this code is tricky and hard to change without breaking
something.

A simpler way to fix this is just always pass in an address buffer of
3 cells to of_irq_parse_raw. You would just need to copy the cells in
of_irq_parse_one() to a temporary buffer.

Rob

Powered by blists - more mailing lists