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(-)
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"):
OF: of_irq_parse_one: dev=/soc@0/picasso/watchdog, index=0
OF: parent=/soc@0/pci@878000000000/gpio0@17,0, intsize=2
OF: intspec=4
OF: of_irq_parse_raw: ipar=/soc@0/pci@878000000000/gpio0@17,0, size=2
OF: -> addrsize=3
==================================================================
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
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
...
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@nokia.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);
dev->irq = irq_create_of_mapping(&oirq);
}
diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
index 6b5d34919c72b..57d946305496a 100644
--- a/drivers/bcma/main.c
+++ b/drivers/bcma/main.c
@@ -175,7 +175,7 @@ static int bcma_of_irq_parse(struct device *parent,
out_irq->args[0] = num;
laddr[0] = cpu_to_be32(core->addr);
- return of_irq_parse_raw(laddr, out_irq);
+ return of_irq_parse_raw(laddr, ARRAY_SIZE(laddr), out_irq);
}
static unsigned int bcma_of_get_irq(struct device *parent,
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index c94203ce65bb3..f3610025a7ed9 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -151,9 +151,29 @@ const __be32 *of_irq_parse_imap_parent(const __be32 *imap, int len, struct of_ph
return imap;
}
+static u32 of_get_address_cells(struct device_node *node) {
+ struct device_node *tnode, *old = NULL;
+ const __be32 *tmp;
+
+ /* Look for this #address-cells. We have to implement the old linux
+ * trick of looking for the parent here as some device-trees rely on it
+ */
+ old = of_node_get(node);
+ do {
+ tmp = of_get_property(old, "#address-cells", NULL);
+ tnode = of_get_parent(old);
+ of_node_put(old);
+ old = tnode;
+ } while (old && tmp == NULL);
+ of_node_put(old);
+ old = NULL;
+ return (tmp == NULL) ? 2 : be32_to_cpu(*tmp);
+}
+
/**
* of_irq_parse_raw - Low level interrupt tree parsing
* @addr: address specifier (start of "reg" property of the device) in be32 format
+ * @addrsize: address cell size ("#address-cells" property of the device (parent))
* @out_irq: structure of_phandle_args updated by this function
*
* This function is a low-level interrupt tree walking function. It
@@ -165,13 +185,13 @@ const __be32 *of_irq_parse_imap_parent(const __be32 *imap, int len, struct of_ph
*
* Return: 0 on success and a negative number on error
*/
-int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
+int of_irq_parse_raw(const __be32 *addr, u32 addrsize, struct of_phandle_args *out_irq)
{
- struct device_node *ipar, *tnode, *old = NULL;
+ struct device_node *ipar, *tnode;
__be32 initial_match_array[MAX_PHANDLE_ARGS];
const __be32 *match_array = initial_match_array;
- const __be32 *tmp, dummy_imask[] = { [0 ... MAX_PHANDLE_ARGS] = cpu_to_be32(~0) };
- u32 intsize = 1, addrsize;
+ const __be32 dummy_imask[] = { [0 ... MAX_PHANDLE_ARGS] = cpu_to_be32(~0) };
+ u32 intsize = 1, ipar_addrsize;
int i, rc = -EINVAL;
#ifdef DEBUG
@@ -201,24 +221,11 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
if (out_irq->args_count != intsize)
goto fail;
- /* Look for this #address-cells. We have to implement the old linux
- * trick of looking for the parent here as some device-trees rely on it
- */
- old = of_node_get(ipar);
- do {
- tmp = of_get_property(old, "#address-cells", NULL);
- tnode = of_get_parent(old);
- of_node_put(old);
- old = tnode;
- } while (old && tmp == NULL);
- of_node_put(old);
- old = NULL;
- addrsize = (tmp == NULL) ? 2 : be32_to_cpu(*tmp);
-
- pr_debug(" -> addrsize=%d\n", addrsize);
+ ipar_addrsize = of_get_address_cells(ipar);
+ pr_debug(" -> addrsize=%d, ipar_addrsize=%d\n", addrsize, ipar_addrsize);
/* Range check so that the temporary buffer doesn't overflow */
- if (WARN_ON(addrsize + intsize > MAX_PHANDLE_ARGS)) {
+ if (WARN_ON(ipar_addrsize + intsize > MAX_PHANDLE_ARGS)) {
rc = -EFAULT;
goto fail;
}
@@ -227,7 +234,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
for (i = 0; i < addrsize; i++)
initial_match_array[i] = addr ? addr[i] : 0;
for (i = 0; i < intsize; i++)
- initial_match_array[addrsize + i] = cpu_to_be32(out_irq->args[i]);
+ initial_match_array[ipar_addrsize + i] = cpu_to_be32(out_irq->args[i]);
/* Now start the actual "proper" walk of the interrupt tree */
while (ipar != NULL) {
@@ -254,7 +261,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
* interrupt-map parsing does not work without a reg
* property when #address-cells != 0
*/
- if (addrsize && !addr) {
+ if (ipar_addrsize && !addr) {
pr_debug(" -> no reg passed in when needed !\n");
goto fail;
}
@@ -274,10 +281,10 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
/* Parse interrupt-map */
match = 0;
- while (imaplen > (addrsize + intsize + 1)) {
+ while (imaplen > (ipar_addrsize + intsize + 1)) {
/* Compare specifiers */
match = 1;
- for (i = 0; i < (addrsize + intsize); i++, imaplen--)
+ for (i = 0; i < (ipar_addrsize + intsize); i++, imaplen--)
match &= !((match_array[i] ^ *imap++) & imask[i]);
pr_debug(" -> match=%d (imaplen=%d)\n", match, imaplen);
@@ -306,7 +313,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
newpar = out_irq->np;
intsize = out_irq->args_count;
- addrsize = (imap - match_array) - intsize;
+ ipar_addrsize = (imap - match_array) - intsize;
if (ipar == newpar) {
pr_debug("%pOF interrupt-map entry to self\n", ipar);
@@ -343,7 +350,7 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
{
struct device_node *p;
const __be32 *addr;
- u32 intsize;
+ u32 addrsize, intsize;
int i, res;
pr_debug("of_irq_parse_one: dev=%pOF, index=%d\n", device, index);
@@ -354,12 +361,13 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
/* Get the reg property (if any) */
addr = of_get_property(device, "reg", NULL);
+ addrsize = of_get_address_cells(device);
/* Try the new-style interrupts-extended first */
res = of_parse_phandle_with_args(device, "interrupts-extended",
"#interrupt-cells", index, out_irq);
if (!res)
- return of_irq_parse_raw(addr, out_irq);
+ return of_irq_parse_raw(addr, addrsize, out_irq);
/* Look for the interrupt parent. */
p = of_irq_find_parent(device);
@@ -389,7 +397,7 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
/* Check if there are any interrupt-map translations to process */
- res = of_irq_parse_raw(addr, out_irq);
+ res = of_irq_parse_raw(addr, addrsize, out_irq);
out:
of_node_put(p);
return res;
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 51e3dd0ea5abe..5d4b2ec5a3b13 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -506,7 +506,7 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
out_irq->args[0] = pin;
laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
laddr[1] = laddr[2] = cpu_to_be32(0);
- rc = of_irq_parse_raw(laddr, out_irq);
+ rc = of_irq_parse_raw(laddr, ARRAY_SIZE(laddr), out_irq);
if (rc)
goto err;
return 0;
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index d6d3eae2f1452..74268e33d7305 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -32,7 +32,8 @@ static inline int of_irq_parse_oldworld(const struct device_node *device, int in
}
#endif /* CONFIG_PPC32 && CONFIG_PPC_PMAC */
-extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
+extern int of_irq_parse_raw(const __be32 *addr, u32 addrsize,
+ struct of_phandle_args *out_irq);
extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
extern int of_irq_to_resource(struct device_node *dev, int index,
struct resource *r);
--
2.42.0On Wed, Aug 7, 2024 at 6:04 AM Stefan Wiehler <stefan.wiehler@nokia.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@0/picasso/watchdog, index=0 > OF: parent=/soc@0/pci@878000000000/gpio0@17,0, intsize=2 > OF: intspec=4 > OF: of_irq_parse_raw: ipar=/soc@0/pci@878000000000/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@nokia.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
> You've missed a bunch of people/lists. Use get_maintainers.pl.
Sorry, indeed, did not think about about PCI...
> 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...
The DTO looks like this:
watchdog {
...
reg = <0x00002064 0x00000028>;
...
interrupt-parent = <&gpio_17_0>;
interrupts = <4 0x8>; // 8 -> IRQ_TYPE_LEVEL_LOW
...
};
And the base DT:
ecam0: pci@878000000000 {
...
#size-cells = <2>;
#address-cells = <3>;
...
gpio_17_0: gpio0@17,0 {
...
reg = <0x8800 0 0 0 0>; /* DEVFN = 0x88 (17:0) */
...
};
...
};
I completely agree it's a bit sketchy, but it's not me who came up with
this ;-) Nevertheless I think other people might run into this issue of
mismatching address sizes as well when no interrupt mapping was intended.
> Note that of_irq_parse_raw() was refactored in 6.10, so your patch is
> not going to apply.
I'm aware of that and have adapted the patch accordingly.
> 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.
Thanks for confirming that it's PCI only and no address size mismatch
should occur. I also was thinking in the direction of checking first if
interrupt mapping is intended and return early otherwise, but was
worried to break something along the way...
> 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.
That indeed sounds like the easiest solution to me; I'll send a new patch
shortly. However I don't understand how you came up with an address
buffer size of 3 - shouldn't it be MAX_PHANDLE_ARGS
(addrsize = MAX_PHANDLE_ARGS and intsize = 0 in the worst case)?
> Please don't send new versions right after the prior version. Give
> people a chance to review.
Sorry about that, I wanted to fix checkpatch because I thought
patch-applied failed due to that, but that seems to be another
issue...
Kind regards,
Stefan
On Thu, Aug 08, 2024 at 09:53:59AM +0200, Stefan Wiehler wrote:
> > You've missed a bunch of people/lists. Use get_maintainers.pl.
>
> Sorry, indeed, did not think about about PCI...
>
> > 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...
>
> The DTO looks like this:
>
> watchdog {
> ...
> reg = <0x00002064 0x00000028>;
> ...
> interrupt-parent = <&gpio_17_0>;
> interrupts = <4 0x8>; // 8 -> IRQ_TYPE_LEVEL_LOW
> ...
> };
>
> And the base DT:
>
> ecam0: pci@878000000000 {
> ...
> #size-cells = <2>;
> #address-cells = <3>;
> ...
> gpio_17_0: gpio0@17,0 {
> ...
> reg = <0x8800 0 0 0 0>; /* DEVFN = 0x88 (17:0) */
> ...
> };
> ...
> };
>
> I completely agree it's a bit sketchy, but it's not me who came up with
> this ;-) Nevertheless I think other people might run into this issue of
> mismatching address sizes as well when no interrupt mapping was intended.
>
> > Note that of_irq_parse_raw() was refactored in 6.10, so your patch is
> > not going to apply.
>
> I'm aware of that and have adapted the patch accordingly.
>
> > 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.
>
> Thanks for confirming that it's PCI only and no address size mismatch
> should occur. I also was thinking in the direction of checking first if
> interrupt mapping is intended and return early otherwise, but was
> worried to break something along the way...
>
> > 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.
>
> That indeed sounds like the easiest solution to me; I'll send a new patch
> shortly. However I don't understand how you came up with an address
> buffer size of 3 - shouldn't it be MAX_PHANDLE_ARGS
> (addrsize = MAX_PHANDLE_ARGS and intsize = 0 in the worst case)?
There is simply no supported case of more than 3 address-cells. If
someone has 4, then their address translation is not going to work and
we won't even get to worrying about interrupts...
Rob
> There is simply no supported case of more than 3 address-cells. If > someone has 4, then their address translation is not going to work and > we won't even get to worrying about interrupts... Oh yes, I was thinking too theoretically... New patch version is out. Kind regards, Stefan
© 2016 - 2025 Red Hat, Inc.