Re: [PATCH v2] block: partitions: Use seq_buf_putc() at 24 places

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds read in sysv68 partition parser leading to a kernel memory leak.
- [Medium] Uninitialized variable 'len' may lead to a NULL pointer dereference or undefined behavior.
--

commit 350b3ea7021ee2238b49cf91963fe54906868307
Author: Markus Elfring <elfring@users.sourceforge.net>

block: partitions: Use seq_buf_putc() at 24 places

Single line breaks should occasionally be put into a sequence buffer.
Thus use the seq_buf_putc() function in these implementations.

The source code was transformed by using the Coccinelle software.

[ ... ]

> diff --git a/block/partitions/of.c b/block/partitions/of.c
> index 53664ea06b654..7803e6f4ecdd9 100644
> --- a/block/partitions/of.c
> +++ b/block/partitions/of.c
> @@ -104,7 +104,7 @@ int of_partition(struct parsed_partitions *state)
>  		slot++;
>  	}
>  
> -	seq_buf_puts(&state->pp_buf, "\n");
> +	seq_buf_putc(&state->pp_buf, '\n');

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential uninitialized
variable bug when looking at the partition validation called just before
this in of_partition().

If a device tree partition node is missing the 'reg' property:

block/partitions/of.c:validate_of_partition() {
    ...
	int len;

	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);

	/* Make sure reg len match the expected addr and size cells */
	if (len / sizeof(*reg) != a_cells + s_cells)
    ...
}

of_get_property() returns NULL and leaves len uninitialized. The code then
divides this uninitialized len by 4 and compares it to expected cell counts.

If the uninitialized stack garbage happens to match, could this proceed to
add_of_partition() where it directly passes the NULL reg pointer to
of_read_number(), unconditionally dereferencing it?

block/partitions/of.c:add_of_partition() {
    ...
	int len;

	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;
    ...
}

[ ... ]

> diff --git a/block/partitions/sysv68.c b/block/partitions/sysv68.c
> index 470e0f9de7be3..31d348ac2dfb3 100644
> --- a/block/partitions/sysv68.c
> +++ b/block/partitions/sysv68.c
> @@ -86,7 +86,7 @@ int sysv68_partition(struct parsed_partitions *state)
>  		}
>  		slot++;
>  	}
> -	seq_buf_puts(&state->pp_buf, "\n");
> +	seq_buf_putc(&state->pp_buf, '\n');

[Severity: High]
This isn't a bug introduced by this patch, but does the loop immediately
preceding this code have an out-of-bounds read issue?

sysv68_partition() reads the number of slices from the untrusted disk
label without validating it against the 512-byte sector buffer bounds:

block/partitions/sysv68.c:sysv68_partition() {
    ...
	slice = (struct slice *)data;
	for (i = 0; i < slices; i++, slice++) {
		if (slot == state->limit)
			break;
		if (be32_to_cpu(slice->nblocks)) {
			put_partition(state, slot,
				be32_to_cpu(slice->blkoff),
				be32_to_cpu(slice->nblocks));
    ...
}

It then iterates up to state->limit (typically 256) times, advancing the
slice pointer. Reading 256 struct slice entries takes 2048 bytes out of a
guaranteed 512-byte sector buffer.

If the 512-byte sector is located near the end of a mapped page, could
reading beyond 512 bytes cross into adjacent physical kernel memory and
leak it via the partition table?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/7360cab3-7a2f-4903-85f1-0f03387b0d88@web.de?part=1