Re: [PATCH] of/address: Fix NULL bus dereference in of_pci_range_parser_one()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] A malformed device tree with zeroed address and size cells causes a division by zero in `of_range_count()`, leading to a kernel panic during boot.

Pre-existing issues:
- [High] Implicit casting of `u32` to `int` in cell count functions allows negative cell counts, resulting in out-of-bounds memory accesses.
--

commit 6df2fc2d90abec7428c67623972602133d111e3d
Author: Carlo Caione <ccaione@baylibre.com>

of/address: Fix NULL bus dereference in of_pci_range_parser_one()

This commit avoids a NULL pointer dereference by rejecting nodes without a
valid bus in parser_init(). It also explicitly guards the DMA limit walk
with a check to of_range_count().

> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index cf4aab11e9b1f..b57b1adbad44a 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c

[ ... ]

> @@ -792,7 +799,7 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>  	int na = parser->na;
>  	int ns = parser->ns;
>  	int np = parser->pna + na + ns;
> -	int busflag_na = parser->bus->flag_cells;
> +	int busflag_na;
>  
>  	if (!range)
>  		return NULL;
> @@ -800,6 +807,8 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>  	if (!parser->range || parser->range + np > parser->end)
>  		return NULL;

[Severity: High]
This is a pre-existing issue, but could a malformed Device Tree with large
cell counts cause an out-of-bounds read here?

If a device tree provides a very large unsigned value like 0x80000000 for
address or size cells, the count becomes negative due to the implicit cast
in of_bus_n_addr_cells():

drivers/of/base.c:of_bus_n_addr_cells() {
    u32 cells;
    ...
    if (!of_property_read_u32(np, "#address-cells", &cells))
        return cells;
}

Because np (pna + na + ns) becomes negative, the bounds check
parser->range + np > parser->end is bypassed. This negative value is then
passed to of_read_number(), which uses a post-decrement loop:

include/linux/of.h:of_read_number() {
    ...
    for (; size--; cell++)
        ...
}

Will this cause the loop to execute excessively and read memory beyond the
allocated buffer?

[ ... ]

> @@ -976,8 +985,8 @@ phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
>  		np = of_root;
>  
>  	ranges = of_get_property(np, "dma-ranges", &len);
> -	if (ranges && len) {
> -		of_dma_range_parser_init(&parser, np);
> +	if (ranges && len && !of_dma_range_parser_init(&parser, np) &&
> +	    of_range_count(&parser)) {

[Severity: High]
Does this new call to of_range_count() trigger a division by zero if the
device tree explicitly zeroes the address and size cells?

If a malformed device tree sets #address-cells and #size-cells to 0 on a
node and its parent, the parser fields na, pna, and ns will all be 0.
When of_range_count() is called, it performs an unchecked division:

include/linux/of_address.h:of_range_count() {
    ...
    return (parser->end - parser->range) / (parser->na + parser->pna + parser->ns);
}

Since the sum of the cells is 0, will this result in a kernel panic during
boot or device hotplug?

>  		for_each_of_range(&parser, &range)
>  			if (range.cpu_addr + range.size > cpu_end)
>  				cpu_end = range.cpu_addr + range.size - 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706095651.48839-1-ccaione@baylibre.com?part=1