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

[PATCH v2] 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. Later on, payload->checksum[0] and
payload->checksum[1] are accessed without verifying that payload->size
is large enough.

Add another check to make sure that payload->size can be safely
accessed, and make sure that the checksum fields are accessible.

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>
---
Changes v1 -> v2: Fixed issues reported by Sashiko

* fix use of wrong variable name payload_len instead of mb_offset
* add checks to ensure payload->size is large enough to hold the checksum values

---
 drivers/md/raid5-cache.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 7b7546bfa21f..4a97846e9030 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) {
+			if (mb_offset + sizeof(struct r5l_payload_data_parity)
+			    > le32_to_cpu(mb->meta_size))
+				goto mismatch;
+			payload_size = le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9);
 			payload_len = sizeof(struct r5l_payload_data_parity) +
-				(sector_t)sizeof(__le32) *
-				(le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
-			if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+					(sector_t)sizeof(__le32) * payload_size;
+			if (mb_offset + payload_len > le32_to_cpu(mb->meta_size) ||
+			    payload_size < 1)
 				goto mismatch;
 			if (r5l_recovery_verify_data_checksum(
 				    log, ctx, page, log_offset,
 				    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_size = le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9);
 			payload_len = sizeof(struct r5l_payload_data_parity) +
-				(sector_t)sizeof(__le32) *
-				(le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
-			if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+					(sector_t)sizeof(__le32) * payload_size;
+			if (mb_offset + payload_len > le32_to_cpu(mb->meta_size) ||
+			    payload_size < conf->max_degraded)
 				goto mismatch;
 			if (r5l_recovery_verify_data_checksum(
 				    log, ctx, page, log_offset,
@@ -2035,6 +2044,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 +2118,9 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
 		if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
 			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);
 			if (mb_offset + payload_len >
@@ -2130,6 +2145,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 v2] md/raid5: validate payload size before calculating payload_len

[sashiko-bot]

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