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: 3213 bytes --]

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()

>  	}
> @@ -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 --]