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

[Martin Wilck <mwilck@suse.com>]

On Mon, 2026-09-07 at 14:29 +0800, yu kuai wrote:
> Hi,
> 
> 在 2026/9/3 2:18, 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 members payload->header.type and
> > 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 v2 -> v3: Fixed another issue reported by Sashiko
> > 
> > * Modify loop condition to make sure the payload->header.type can
> > be accessed
> > 
> > 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 | 34 ++++++++++++++++++++++++++--------
> >   1 file changed, 26 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> > index 7b7546bfa21f..031a89faddae 100644
> > --- a/drivers/md/raid5-cache.c
> > +++ b/drivers/md/raid5-cache.c
> > @@ -2001,27 +2001,36 @@
> > 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 + sizeof(struct r5l_payload_header) <
> > le32_to_cpu(mb->meta_size)) {
> 
> Offset can't be meta_size, however, is it possible that the remaining
> space is exactly one
> payload size? If so I think <= should be used here.

Right. But practically speaking, as this must be either a struct
r5l_payload_data_parity or a struct r5l_payload_flush, and both contain
additional elements besides the header, if "==" applies here, we will
run into one of the following error conditions later, because the size
element won't be accessible.

Anyway, for the sake of correctness, I'll change it to "<=".

> 
> > +		uint32_t payload_size;
> 
> checkpatch report u32 is prefered.

Strange, it did not for me. I'll change it.

Thanks,
Martin


-- 
Dr. Martin Wilck <mwilck@suse.com>
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg,
Germany
Geschäftsführer: Jochen Jaser, Andrew McDonald (HRB 36809,AG Nürnberg)