Re: [PATCH] md/raid5: validate payload size before calculating payload_len

[sashiko-bot@kernel.org]

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

New issues:
- [High] Uninitialized stack variable used in metadata bounds check

Pre-existing issues:
- [High] Out-of-bounds read when accessing flexible array members without sufficient payload size
--

commit b4a013d747a0232b449bf5e128c73f08e565db4a
Author: Martin Wilck <martin.wilck@suse.com>
Subject: md/raid5: validate payload size before calculating payload_len

This patch introduces additional bounds checks in the journal recovery logic
to ensure that metadata block bounds are respected before accessing variable
length payload sizes. It adds safety checks before accessing payload->size
to prevent out-of-bounds reads on corrupted metadata.

> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 7b7546bfa21fb..accf8b472082c 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -2008,6 +2008,9 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
>  		payload_flush = (void *)mb + mb_offset;
>  
>  		if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
> +			if (mb_offset + sizeof(struct r5l_payload_data_parity)
> +			    > le32_to_cpu(mb->meta_size))
> +				goto mismatch;
>  			payload_len = sizeof(struct r5l_payload_data_parity) +
>  				(sector_t)sizeof(__le32) *
>  				(le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
> @@ -2018,6 +2021,9 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
>  				    payload->checksum[0]) < 0)

[Severity: High]
This is a pre-existing issue, but is it possible that this access to
checksum[0] could result in an out-of-bounds memory read?

If payload->size is 0, payload_len could correctly match the base structure
size without errors. However, reading the flexible array member checksum[0]
without validating that payload->size > 0 could read outside the allocated
meta_page.

>  				goto mismatch;
>  		} else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY) {

[ ... ]

> @@ -2035,6 +2041,9 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
>  				    payload->checksum[1]) < 0)

[Severity: High]
This is also a pre-existing issue, but does this similarly read out of bounds
if payload->size is not large enough to hold a second parity checksum?

Like the data payload case, this might read past the bounds of the 
allocated meta_page if the flexible array member size isn't validated
for RAID6 parity requirements.

>  				goto mismatch;
>  		} else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {

[ ... ]

> @@ -2106,6 +2115,9 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
>  		if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
>  			int i, count;
>  
> +			if (payload_len + sizeof(struct r5l_payload_flush) >
> +			    le32_to_cpu(mb->meta_size))
> +				return -EINVAL;

[Severity: High]
Does this code use payload_len before it is initialized?

In r5c_recovery_analyze_meta_block(), payload_len is declared as a local
variable at the start of the while loop, but it is not initialized before
this check.

Should this evaluate mb_offset instead of payload_len? Using an
uninitialized stack variable here could result in rejecting valid metadata
and aborting RAID journal recovery.

>  			payload_len = sizeof(struct r5l_payload_flush) +
>  				(sector_t)le32_to_cpu(payload_flush->size);
>  			if (mb_offset + payload_len >

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902160603.218621-1-mwilck@suse.com?part=1