Re: [RFC PATCH 1/3] soc: renesas: rz-sysc: Configure AOF registers from dma-ranges

[sashiko-bot@kernel.org]

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

New issues:
- [High] The driver incorrectly uses the parent's `#address-cells` and `#size-cells` to parse all fields of the `dma-ranges` property, which fails if the child and parent buses have different cell sizes.
- [High] An integer overflow in the `cell_sz` calculation allows a maliciously crafted Device Tree to bypass property length checks, resulting in a massive out-of-bounds read.
- [Medium] `rz_sysc_setup_aof()` globally traverses all `simple-bus` nodes with a `dma-ranges` property in the system, emitting warnings for unrelated buses.
- [Medium] The target physical address validation silently truncates addresses that exceed the maximum addressable width instead of returning an error.
- [Medium] An empty `dma-ranges` property incorrectly programs all hardware windows to physical address 0 instead of establishing a 1:1 mapping.
- [Medium] The check to ensure the described child range does not exceed the master's 4 GiB address space is completely bypassed because bits above 4 GiB are masked out before validation.

Pre-existing issues:
- [High] Missing `soc_device_unregister()` for the SoC device created in `rz_sysc_soc_init()`. If `rz_sysc_probe()` fails, devres cleans up `soc_dev_attr` leading to a Use-After-Free when sysfs is accessed, and the `soc_device` is permanently leaked.
--

commit dfb512e475d4fea533f55a96af47e3ef4b23f122
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

soc: renesas: rz-sysc: Configure AOF registers from dma-ranges

This patch adds generic infrastructure to configure the Address Offset Function
(AOF) registers from the firmware-provided dma-ranges property. It sets up
memory windows for masters that generate 32-bit addresses but need to access
memory above the 4 GiB boundary. The appropriate SYS_AOF registers are
programmed by parsing and validating the dma-ranges.

> diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c
> index 0e92c415d4bef..2dae4a0de99bc 100644
> --- a/drivers/soc/renesas/rz-sysc.c
> +++ b/drivers/soc/renesas/rz-sysc.c
[ ... ]
> @@ -28,6 +29,121 @@ struct rz_sysc {
>  	struct device *dev;
>  };
>  
[ ... ]
> +static int rz_aof_parse_bus(struct rz_sysc *sysc,
> +			    const struct rz_sysc_soc_id_init_data *soc_data,
> +			    struct device_node *bus_np)
> +{
[ ... ]
> +	naddr = of_n_addr_cells(bus_np);
> +	nsize = of_n_size_cells(bus_np);
> +	cell_sz = (naddr * 2 + nsize) * sizeof(u32);

[Severity: High]
Does this code fetch the wrong cell sizes? The standard dma-ranges format
requires the child address to be parsed using the child's address-cells,
which would be of_bus_n_addr_cells(bus_np). of_n_addr_cells(bus_np) and
of_n_size_cells(bus_np) fetch the parent's cell sizes, which corrupts the
subsequent property extraction if the child and parent have different sizes.

Also, is there an integer overflow risk here? 

of_n_addr_cells() can return an unvalidated 32-bit integer directly from
the device tree. The cell_sz calculation uses a signed 32-bit integer for
naddr, and the result expands to a 64-bit unsigned size_t but is truncated
when assigned to the int cell_sz. 

Could this truncation bypass property length checks later?

> +
> +	ranges = of_get_property(bus_np, "dma-ranges", &len);
> +	if (!ranges || !cell_sz || len % cell_sz) {
> +		dev_err(sysc->dev, "%pOF: malformed dma-ranges\n", bus_np);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Will this handle an empty dma-ranges property correctly? 

An empty property is valid and indicates a 1:1 mapping, returning len = 0.
The validation len % cell_sz succeeds for 0, so the loop is skipped and
reg_val remains 0. Writing 0 configures all windows to point to physical
address 0, which breaks the 1:1 mapping.

> +	nentries = len / cell_sz;
> +
> +	for (i = 0; i < nentries; i++) {
> +		const __be32 *p = ranges + i * (naddr * 2 + nsize);
> +		u64 child_addr = of_read_number(p, naddr);
> +		u64 parent_addr = of_read_number(p + naddr, naddr);

[Severity: High]
Is it intentional to use the parent's naddr cell count to extract both the
child and parent addresses? 

Additionally, if a massive unvalidated naddr bypassed the length check earlier
due to overflow, won't of_read_number(p, naddr) cause a massive out-of-bounds
read and kernel panic?

> +		u64 size = of_read_number(p + 2 * naddr, nsize);
> +		unsigned int nseg = DIV_ROUND_UP_ULL(size, SZ_1G);
> +		unsigned int sel0 = (child_addr >> 30) & 0x3;

[Severity: Medium]
Does this mask out bits above 4 GiB? 

By using & 0x3, the overflow bits are discarded, so sel0 wraps around. This
completely bypasses the overflow check (if (sel > 3)) later in the loop,
causing invalid configurations to be silently applied to the wrong windows.

> +		u64 tgt0 = parent_addr >> 30;
> +		unsigned int s;
> +
> +		if (child_addr & (SZ_1G - 1) || parent_addr & (SZ_1G - 1)) {
> +			dev_err(sysc->dev, "%pOF: entry %d not 1GiB aligned\n", bus_np, i);
> +			return -EINVAL;
> +		}
> +
> +		for (s = 0; s < nseg; s++) {
> +			unsigned int sel = sel0 + s;
> +			u64 tgt = tgt0 + s;
> +
> +			if (sel > 3) {
> +				dev_err(sysc->dev, "%pOF: entry %d overflows the 4 fields\n",
> +					bus_np, i);
> +				return -EINVAL;
> +			}
> +			if (unit->addr_bits == 35 && (tgt & BIT(5))) {
> +				dev_err(sysc->dev,
> +					"%pOF: target 0x%llx needs bit[5], unit is 35-bit only\n",
> +					bus_np, tgt << 30);
> +				return -EINVAL;
> +			}
> +			reg_val &= ~(0x3fu << (sel * 8));
> +			reg_val |= (tgt & 0x3f) << (sel * 8);

[Severity: Medium]
Does this silently truncate target addresses >= 64 GiB? 

If parent_addr is >= 64 GiB (tgt >= 64), the 6th bit is set, meaning
tgt & BIT(5) is 0, bypassing the validation check entirely. The target is then
truncated to 6 bits by (tgt & 0x3f) << (sel * 8) instead of returning an error.

> +		}
> +	}
> +
> +	writel(reg_val, sysc->base + SYS_AOF_OFFSET(soc_data->aof_base, unit->index));
> +
> +	return 0;
> +}
> +
> +static void rz_sysc_setup_aof(struct rz_sysc *sysc,
> +			      const struct rz_sysc_soc_id_init_data *soc_data)
> +{
> +	struct device_node *np;
> +
> +	if (!soc_data->aof_units || !soc_data->num_aof_units)
> +		return;
> +
> +	for_each_node_with_property(np, "dma-ranges") {
> +		if (!of_device_is_compatible(np, "simple-bus"))
> +			continue;
> +
> +		if (rz_aof_parse_bus(sysc, soc_data, np))
> +			dev_warn(sysc->dev, "AOF setup failed for %pOF\n", np);
> +	}
> +}

[Severity: Medium]
Does this loop emit false-positive warnings for unrelated buses? 

Searching globally for any simple-bus with a dma-ranges property will match
valid buses that have nothing to do with AOF, causing rz_aof_lookup() to fail
and spitting out warning logs.

[ ... ]
> @@ -137,6 +253,8 @@ static int rz_sysc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	rz_sysc_setup_aof(sysc, data->soc_id_init_data);
> +
>  	regmap_cfg->name = "rz_sysc_regs";
>  	regmap_cfg->reg_bits = 32;
>  	regmap_cfg->reg_stride = 4;

[Severity: High]
This is a pre-existing issue, but does rz_sysc_soc_init() leak the SoC device
and cause a use-after-free?

rz_sysc_soc_init() registers an SoC device using soc_device_register() with a
devres-managed soc_dev_attr structure, and discards the returned pointer.

If rz_sysc_probe() fails later (e.g., in devm_regmap_init_mmio()), devres
cleans up soc_dev_attr, leaving the registered soc_device with a dangling
pointer. Accessing sysfs for this device will then cause a use-after-free, and
the soc_device memory is leaked because soc_device_unregister() is never called.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909193640.8069-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=1