Re: [PATCH] md/raid5: validate journal checksum slots during recovery

Hi,

在 2026/7/8 21:35, Guangshuo Li 写道:
> The change referenced by the Fixes tag added payload length validation
> before accessing journal metadata during raid5-cache recovery.
>
> However, the DATA and PARITY payload length is computed from the on-disk
> size field without ensuring that the checksum slots read later are
> actually present. struct r5l_payload_data_parity ends with a flexible
> checksum array, so sizeof(struct r5l_payload_data_parity) does not cover
> any checksum entries.
>
> A corrupted journal can set a DATA payload size smaller than one page.
> The computed checksum count then becomes zero and the payload length only
> covers the fixed header, but recovery still reads checksum[0]. For RAID6
> PARITY payloads, recovery also reads checksum[1], so the payload must
> cover two checksum entries.
>
> Make the validated DATA and PARITY payload length include the checksum
> entries that recovery may read. Also make sure fixed payload headers are
> present before reading their fields.
>
> Fixes: b0cc3ae97e89 ("md/raid5: validate payload size before accessing journal metadata")

Does the problem this patch fixes introduced by above fix tag? Or it's just a
new validation, if so please replace with a real fix tag.

> Signed-off-by: Guangshuo Li <[email protected]>
> ---
>   drivers/md/raid5-cache.c | 108 +++++++++++++++++++++++++++++----------
>   1 file changed, 82 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 7b7546bfa21f..4aef692182b4 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -1980,6 +1980,29 @@ r5l_recovery_verify_data_checksum(struct r5l_log *log,
>   	return (le32_to_cpu(log_checksum) == checksum) ? 0 : -EINVAL;
>   }
>   
> +static sector_t r5l_recovery_payload_data_parity_len(struct r5conf *conf,
> +		const struct r5l_payload_data_parity *payload, bool parity)
> +{
> +	unsigned int nr_csum;
> +	unsigned int min_csum = 1;
> +
> +	/*
> +	 * The payload size determines how many checksum entries are stored,
> +	 * but recovery always reads checksum[0].  For RAID6 parity payloads
> +	 * it also reads checksum[1] for Q.  Make the validated payload length
> +	 * cover every checksum entry that will be read below.
> +	 */
> +	nr_csum = le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9);
> +
> +	if (parity && conf->max_degraded == 2)
> +		min_csum = 2;
> +	if (nr_csum < min_csum)
> +		nr_csum = min_csum;
> +
> +	return sizeof(*payload) + (sector_t)sizeof(__le32) * nr_csum;

Please use struct_size()

> +}
> +
> +

Two blank line here. Please run checkpatch before submission.

>   /*
>    * before loading data to stripe cache, we need verify checksum for all data,
>    * if there is mismatch for any data page, we drop all data in the mata block
> @@ -1992,6 +2015,7 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
>   	struct r5conf *conf = mddev->private;
>   	struct r5l_meta_block *mb = page_address(ctx->meta_page);
>   	sector_t mb_offset = sizeof(struct r5l_meta_block);
> +	sector_t meta_size = le32_to_cpu(mb->meta_size);
>   	sector_t log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
>   	struct page *page;
>   	struct r5l_payload_data_parity *payload;
> @@ -2001,28 +2025,42 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
>   	if (!page)
>   		return -ENOMEM;
>   
> -	while (mb_offset < le32_to_cpu(mb->meta_size)) {
> +	while (mb_offset < meta_size) {
>   		sector_t payload_len;
> +		u16 type;
>   
>   		payload = (void *)mb + mb_offset;
>   		payload_flush = (void *)mb + mb_offset;
>   
> -		if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
> -			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))
> +		if (mb_offset + sizeof(payload->header) > meta_size)
> +			goto mismatch;
> +
> +		type = le16_to_cpu(payload->header.type);
> +
> +		if (type == R5LOG_PAYLOAD_DATA) {
> +			if (mb_offset + sizeof(*payload) > meta_size)
>   				goto mismatch;
> +
> +			payload_len = r5l_recovery_payload_data_parity_len(conf,
> +									  payload,
> +									  false);
> +			if (payload_len > meta_size - mb_offset)
> +				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) {
> -			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))
> +		} else if (type == R5LOG_PAYLOAD_PARITY) {
> +			if (mb_offset + sizeof(*payload) > meta_size)
>   				goto mismatch;
> +
> +			payload_len = r5l_recovery_payload_data_parity_len(conf,
> +									  payload,
> +									  true);
> +			if (payload_len > meta_size - mb_offset)
> +				goto mismatch;
> +
>   			if (r5l_recovery_verify_data_checksum(
>   				    log, ctx, page, log_offset,
>   				    payload->checksum[0]) < 0)
> @@ -2034,15 +2072,18 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
>   						 BLOCK_SECTORS),
>   				    payload->checksum[1]) < 0)
>   				goto mismatch;
> -		} else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
> +		} else if (type == R5LOG_PAYLOAD_FLUSH) {
> +			if (mb_offset + sizeof(*payload_flush) > 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))
> +			if (payload_len > meta_size - mb_offset)
>   				goto mismatch;
>   		} else /* not R5LOG_PAYLOAD_DATA/PARITY/FLUSH */
>   			goto mismatch;
>   
> -		if (le16_to_cpu(payload->header.type) != R5LOG_PAYLOAD_FLUSH) {
> +		if (type != R5LOG_PAYLOAD_FLUSH) {
>   			log_offset = r5l_ring_add(log, log_offset,
>   						  le32_to_cpu(payload->size));
>   		}
> @@ -2075,7 +2116,8 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
>   	struct r5l_meta_block *mb;
>   	struct r5l_payload_data_parity *payload;
>   	struct r5l_payload_flush *payload_flush;
> -	int mb_offset;
> +	sector_t mb_offset;
> +	sector_t meta_size;
>   	sector_t log_offset;
>   	sector_t stripe_sect;
>   	struct stripe_head *sh;
> @@ -2094,22 +2136,31 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
>   
>   	mb = page_address(ctx->meta_page);
>   	mb_offset = sizeof(struct r5l_meta_block);
> +	meta_size = le32_to_cpu(mb->meta_size);
>   	log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
>   
> -	while (mb_offset < le32_to_cpu(mb->meta_size)) {
> +	while (mb_offset < meta_size) {
>   		sector_t payload_len;
> +		u16 type;
>   		int dd;
>   
>   		payload = (void *)mb + mb_offset;
>   		payload_flush = (void *)mb + mb_offset;
>   
> -		if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
> +		if (mb_offset + sizeof(payload->header) > meta_size)
> +			return -EINVAL;
> +
> +		type = le16_to_cpu(payload->header.type);
> +
> +		if (type == R5LOG_PAYLOAD_FLUSH) {
>   			int i, count;
>   
> +			if (mb_offset + sizeof(*payload_flush) > meta_size)
> +				return -EINVAL;
> +
>   			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))
> +			if (payload_len > meta_size - mb_offset)
>   				return -EINVAL;
>   
>   			count = le32_to_cpu(payload_flush->size) / sizeof(__le64);
> @@ -2130,13 +2181,18 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
>   		}
>   
>   		/* DATA or PARITY payload */
> -		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))
> +		if (type != R5LOG_PAYLOAD_DATA && type != R5LOG_PAYLOAD_PARITY)
> +			return -EINVAL;
> +
> +		if (mb_offset + sizeof(*payload) > meta_size)
> +			return -EINVAL;
> +
> +		payload_len = r5l_recovery_payload_data_parity_len(conf, payload,
> +					type == R5LOG_PAYLOAD_PARITY);
> +		if (payload_len > meta_size - mb_offset)
>   			return -EINVAL;
>   
> -		stripe_sect = (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) ?
> +		stripe_sect = (type == R5LOG_PAYLOAD_DATA) ?
>   			raid5_compute_sector(
>   				conf, le64_to_cpu(payload->location), 0, &dd,
>   				NULL)
> @@ -2183,7 +2239,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
>   			list_add_tail(&sh->lru, cached_stripe_list);
>   		}
>   
> -		if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
> +		if (type == R5LOG_PAYLOAD_DATA) {
>   			if (!test_bit(STRIPE_R5C_CACHING, &sh->state) &&
>   			    test_bit(R5_Wantwrite, &sh->dev[sh->pd_idx].flags)) {
>   				r5l_recovery_replay_one_stripe(conf, sh, ctx);
> @@ -2191,7 +2247,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
>   			}
>   			r5l_recovery_load_data(log, sh, ctx, payload,
>   					       log_offset);
> -		} else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY)
> +		} else if (type == R5LOG_PAYLOAD_PARITY)
>   			r5l_recovery_load_parity(log, sh, ctx, payload,
>   						 log_offset);
>   		else

-- 
Thanks,
Kuai