Re: [PATCH v5 1/9] block: partitions: of: Skip child nodes without reg property

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The device node reference for `partitions_np` is acquired but leaked on both the success and incompatibility early-return paths.
- [High] Potential NULL pointer dereference in `strscpy()` if a partition node lacks both `label` and `name` properties.
- [High] Malicious device tree properties `#address-cells` and `#size-cells` can cause integer underflow, bypassing the length check and leading to massive out-of-bounds reads.
--

commit a6b306502d7065c56b83fd73a46be12067fb56e3
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

block: partitions: of: Skip child nodes without reg property
    
Child nodes of a fixed-partitions node are not necessarily partition
entries, for example an nvmem-layout node has no reg property. The
current code passes a NULL reg pointer and uninitialized len to the
length check, which can result in a kernel panic or silent failure to
register any partitions.

> diff --git a/block/partitions/of.c b/block/partitions/of.c
> index c22b606610981..534e02a9d85f6 100644
> --- a/block/partitions/of.c
> +++ b/block/partitions/of.c
> @@ -15,6 +15,10 @@ static int validate_of_partition(struct device_node *np, int slot)
>  	int a_cells = of_n_addr_cells(np);
>  	int s_cells = of_n_size_cells(np);
>  
> +	/* Skip nodes without a reg property (e.g. nvmem-layout) */
> +	if (!reg)
> +		return 1;
> +
>  	/* Make sure reg len match the expected addr and size cells */
>  	if (len / sizeof(*reg) != a_cells + s_cells)
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but could malicious device tree properties
cause an integer underflow here?

The functions of_n_addr_cells() and of_n_size_cells() return signed integers.
If a device tree specifies a large unsigned value like 0xffffffff, these
functions will return -1.

The validation check above can be bypassed if the sum of negative and positive
cell counts equals the length ratio.

When the negative size is later passed to of_read_number(reg, a_cells) in
add_of_partition(), the internal while (size--) loop underflows, causing a
massive out-of-bounds read and panic.

[ ... ]

> @@ -97,9 +102,10 @@ int of_partition(struct parsed_partitions *state)
>  			break;
>  		}
>  
> -		add_of_partition(state, slot, np);
> -
> -		slot++;
> +		if (of_property_present(np, "reg")) {
> +			add_of_partition(state, slot, np);

[Severity: High]
This isn't a bug introduced by this patch, but does add_of_partition() have a
NULL pointer dereference risk?

Inside add_of_partition(), partname is populated by checking for "label"
and falling back to "name". If a node omits both properties, of_get_property()
returns NULL:

block/partitions/of.c:add_of_partition() {
    ...
	partname = of_get_property(np, "label", &len);
	if (!partname)
		partname = of_get_property(np, "name", &len);
	strscpy(info->volname, partname, sizeof(info->volname));
    ...
}

Will passing a NULL partname to strscpy() cause a kernel panic?

> +			slot++;
> +		}
>  	}
>  
>  	seq_buf_puts(&state->pp_buf, "\n");

[Severity: High]
This is a pre-existing issue, but does this function leak the partitions_np
device node reference?

A reference is acquired via of_node_get(ddev->of_node) at the start of
of_partition().

If the node is not compatible with "fixed-partitions", the function returns 0
early without calling of_node_put(partitions_np).

Similarly, on this successful parsing path, it returns 1 without releasing
the reference.

Could this lead to memory exhaustion or block the release of dynamically
allocated device nodes every time partitions are rescanned?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-block-as-nvmem-v5-0-95e0b30fff90@oss.qualcomm.com?part=1