Re: [PATCH 3/3] fpga: microchip-spi: add bounds checks in mpf_ops_parse_header()

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3636 bytes --]

On Thu, Apr 02, 2026 at 04:03:19PM +0100, Conor Dooley wrote:
> On Thu, Apr 02, 2026 at 06:54:46AM -0600, Sebastian Alba Vives wrote:
> > From: Sebastian Josue Alba Vives <sebasjosue84@gmail.com>
> > 
> > mpf_ops_parse_header() reads several fields from the bitstream file
> > and uses them as offsets and sizes without validating them against the
> > buffer size, leading to multiple out-of-bounds read vulnerabilities:
> > 
> > 1. When header_size (u8 from file) is 0, the expression
> >    *(buf + header_size - 1) reads one byte before the buffer.
> > 
> > 2. In the block lookup loop, block_id_offset and block_start_offset
> >    advance by MPF_LOOKUP_TABLE_RECORD_SIZE (9) each iteration with
> >    blocks_num (u8) controlling the count. With a small buffer, these
> >    offsets exceed count, causing OOB reads via get_unaligned_le32().
> > 
> > 3. components_size_start (from file) and component_size_byte_num
> >    (derived from components_num, u16 from file) are used as offsets
> >    into buf without validation, allowing arbitrary OOB reads.
> > 
> > Add bounds checks for all three cases: reject header_size of 0,
> > validate offsets in the block lookup loop, and validate the component
> > size read offset.
> > 
> > Signed-off-by: Sebastian Alba Vives <sebasjosue84@gmail.com>
> 
> Fixes: 5f8d4a9008307 ("fpga: microchip-spi: add Microchip MPF FPGA manager")
> CC: stable@vger.kernel.org
> 
> > ---
> >  drivers/fpga/microchip-spi.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> > index 6134cea..7954dd0 100644
> > --- a/drivers/fpga/microchip-spi.c
> > +++ b/drivers/fpga/microchip-spi.c
> > @@ -116,7 +116,7 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
> >  	}
> >  
> >  	header_size = *(buf + MPF_HEADER_SIZE_OFFSET);
> > -	if (header_size > count) {
> > +	if (!header_size || header_size > count) {
> >  		info->header_size = header_size;
> >  		return -EAGAIN;
> 
> Hmm, looking at this while feeling less ill, is this actually right?
> The function returns EAGAIN to retry with a bigger buffer, but if
> header_size is zero, increasing the buffer size will not change that
> fact, and it'll go around forever? See the user in
> fpga_mgr_parse_header_sg()

Also, can we trust that count is even big enough for us to read at an
offset of MPF_HEADER_SIZE_OFFSET (24)? Should that be checked before we
even attempt looking at header_size?

> 
> >  	}
> > @@ -139,6 +139,10 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
> >  	bitstream_start = 0;
> >  
> >  	while (blocks_num--) {
> > +		if (block_id_offset >= count ||
> > +		    block_start_offset + sizeof(u32) > count)
> > +			return -EINVAL;
> 
> And here, why doesn't it return EAGAIN? Isn't the provided section of
> the image just too small, so we want to be provided more of it to
> actually complete header parsing?
> 
> Cheers,
> Conor.
> 
> > +
> >  		block_id = *(buf + block_id_offset);
> >  		block_start = get_unaligned_le32(buf + block_start_offset);
> >  
> > @@ -183,6 +187,10 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
> >  		component_size_byte_off =
> >  			(i * MPF_BITS_PER_COMPONENT_SIZE) % BITS_PER_BYTE;
> >  
> > +		if (components_size_start + component_size_byte_num +
> > +		    sizeof(u32) > count)
> > +			return -EINVAL;
> > +
> >  		component_size = get_unaligned_le32(buf +
> >  						    components_size_start +
> >  						    component_size_byte_num);
> > -- 
> > 2.43.0
> > 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]