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

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

[Martin Wilck]

Commit b0cc3ae97e89 ("md/raid5: validate payload size before accessing
journal metadata") introduced a bounds check to verify that a given payload
fits into the current metadata block. In order to calculate the payload
size, it has to access the member payload->size, the offset of which may
already be past the end of the metadata block, because the loop condition
only checks that the first byte of the payload is inside, and the payload
entries have variable sizes.

Add another check to make sure that payload->size can be safely
accessed.

This issue has been found by AI (gemma4, Gemini) during a backport
review.

Cc: Junrui Luo <moonafterrain@outlook.com>
Cc: stable@vger.kernel.org
Fixes: b0cc3ae97e89 ("md/raid5: validate payload size before accessing journal metadata")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/md/raid5-cache.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 7b7546bfa21f..accf8b472082 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)
 				goto mismatch;
 		} else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY) {
+			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));
@@ -2035,6 +2041,9 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
 				    payload->checksum[1]) < 0)
 				goto mismatch;
 		} else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
+			if (mb_offset + sizeof(struct r5l_payload_flush)
+			    > le32_to_cpu(mb->meta_size))
+				goto mismatch;
 			payload_len = sizeof(struct r5l_payload_flush) +
 				(sector_t)le32_to_cpu(payload_flush->size);
 			if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
@@ -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;
 			payload_len = sizeof(struct r5l_payload_flush) +
 				(sector_t)le32_to_cpu(payload_flush->size);
 			if (mb_offset + payload_len >
@@ -2130,6 +2142,9 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
 		}
 
 		/* DATA or PARITY payload */
+		if (mb_offset + sizeof(struct r5l_payload_data_parity) >
+		    le32_to_cpu(mb->meta_size))
+			return -EINVAL;
 		payload_len = sizeof(struct r5l_payload_data_parity) +
 			(sector_t)sizeof(__le32) *
 			(le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
-- 
2.51.0

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

[sashiko-bot]

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