Re: [PATCH v11 2/7] block: partitions: of: Attach partition fwnode to the block device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] block: of_partition() TOCTOU race causing NULL pointer deref
- [Medium] block: partition extended minor leaked on add_partition error path
--

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

block: partitions: of: Attach partition fwnode to the block device

The OF partition parser reads offset, size and label from each
"fixed-partitions" child node but does not associate that node with the
resulting partition block device. As a result a partition has no
of_node, unlike the whole-disk device which gets its firmware node via
add_disk_fwnode(). This commit carries the partition's device tree node
through the parser and attaches it to the partition's block device.

> diff --git a/block/partitions/core.c b/block/partitions/core.c
> index b5c59b79ca7cb..eee954df1ac6e 100644
> --- a/block/partitions/core.c
> +++ b/block/partitions/core.c

[ ... ]

> @@ -343,6 +356,7 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
>  	pdev->class = &block_class;
>  	pdev->type = &part_type;
>  	pdev->parent = ddev;
> +	device_set_node(pdev, fwnode_handle_get(fwnode));
>  
>  	/* in consecutive minor range? */
>  	if (bdev_partno(bdev) < disk->minors) {

[Severity: Medium]
In add_partition(), if the partition number exceeds disk->minors,
blk_alloc_ext_minor() allocates an extended minor IDR entry.

If an error occurs after this allocation but before bdev_add() (such as
kmemdup() or device_add() failing), the error path calls put_device(pdev).
This triggers the following call chain:

put_device() -> part_release() -> bdev_drop() -> iput() -> bdev_free_inode()

bdev_free_inode() checks MAJOR(bdev->bd_dev) == BLOCK_EXT_MAJOR to free the
minor. Because bdev->bd_dev is not assigned until bdev_add() later in the
function, does this permanently leak the IDR entry for the extended minor on
failure?

> diff --git a/block/partitions/of.c b/block/partitions/of.c
> index 53664ea06b654..4fc7c80c407de 100644
> --- a/block/partitions/of.c
> +++ b/block/partitions/of.c

[ ... ]

> @@ -62,6 +63,8 @@ static void add_of_partition(struct parsed_partitions *state, int slot,
>  		partname = of_get_property(np, "name", &len);
>  	strscpy(info->volname, partname, sizeof(info->volname));
>  
> +	state->parts[slot].fwnode = fwnode_handle_get(of_fwnode_handle(np));
> +
>  	seq_buf_printf(&state->pp_buf, "(%s)", info->volname);
>  }

[Severity: High]
In of_partition(), child nodes are parsed using two separate loops. The
first loop calls validate_of_partition(), which ensures the "reg" property
exists. The second loop then passes nodes to add_of_partition().

If a Device Tree overlay asynchronously inserts a new child node without a
"reg" property between the validation loop and the processing loop, does
this new node skip validation?

If this unvalidated node is passed to add_of_partition():

block/partitions/of.c:add_of_partition() {
	...
	const __be32 *reg = of_get_property(np, "reg", &len);
	int a_cells = of_n_addr_cells(np);
	int s_cells = of_n_size_cells(np);

	/* Convert bytes to sector size */
	u64 offset = of_read_number(reg, a_cells) / SECTOR_SIZE;
	...
}

Will this result in a NULL pointer dereference in of_read_number() when it
attempts to access the NULL reg pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-block-as-nvmem-v11-0-f890ac74f87d@oss.qualcomm.com?part=2