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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds read of `payload->header.type` across page boundary
--

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

This commit introduces a bounds check to verify that a payload fits into the
current metadata block. It ensures that the payload size and checksum fields
can be safely accessed without reading past the end of the metadata block.

> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 7b7546bfa21fb..4a97846e9030b 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -2002,26 +2002,35 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
>  		return -ENOMEM;
>  
>  	while (mb_offset < le32_to_cpu(mb->meta_size)) {
> +		uint32_t payload_size;
>  		sector_t payload_len;
>  
>  		payload = (void *)mb + mb_offset;
>  		payload_flush = (void *)mb + mb_offset;
>  
>  		if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {

[Severity: High]
This isn't a bug introduced by this patch, but could this read of
payload->header.type cause an out-of-bounds access across a page boundary?

In r5l_recovery_verify_data_checksum_for_mb(), if a corrupted or maliciously
crafted RAID journal log configures a payload size such that mb_offset
evaluates exactly to mb->meta_size - 1 (e.g. 4095), the loop condition
mb_offset < le32_to_cpu(mb->meta_size) will evaluate to true.

The subsequent read of le16_to_cpu(payload->header.type) will try to access
2 bytes starting at offset 4095. Does this mean it would attempt to read
offset 4096, which is out-of-bounds of the meta_page?

> +			if (mb_offset + sizeof(struct r5l_payload_data_parity)
> +			    > le32_to_cpu(mb->meta_size))
> +				goto mismatch;

[ ... ]

> @@ -2106,6 +2118,9 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
>  		if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {

[Severity: High]
This is also a pre-existing issue, but does the same out-of-bounds read flaw
exist here in r5c_recovery_analyze_meta_block() when analyzing payloads?

If mb_offset is just 1 byte shy of mb->meta_size, would reading the 2-byte
payload->header.type field read past the end of the metadata page before the
newly added bounds checks are reached?

>  			int i, count;
>  
> +			if (mb_offset + sizeof(struct r5l_payload_flush) >
> +			    le32_to_cpu(mb->meta_size))
> +				return -EINVAL;
>  			payload_len = sizeof(struct r5l_payload_flush) +
>  				(sector_t)le32_to_cpu(payload_flush->size);

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