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

[Conor Dooley]

On Thu, Apr 02, 2026 at 09:37:52AM -0600, Sebastian Alba Vives wrote:
> From: Sebastian Josue Alba Vives <sebasjosue84@xxxxxxxxx>
> 
> 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. There is no check that count is large enough to read header_size
>    at MPF_HEADER_SIZE_OFFSET (24). Add a minimum count check.
> 
> 2. When header_size (u8 from file) is 0, the expression
>    *(buf + header_size - 1) reads one byte before the buffer.
>    Return -EINVAL since retrying with a larger buffer cannot fix
>    a zero header_size.
> 
> 3. 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().
>    Return -EAGAIN since a larger buffer may resolve the issue.
> 
> 4. 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 four cases.
> 
> Fixes: 5f8d4a9008307 ("fpga: microchip-spi: add Microchip MPF FPGA manager")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Sebastian Alba Vives <sebasjosue84@xxxxxxxxx>
> ---
>  drivers/fpga/microchip-spi.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c
> index 6134cea..00fa2d6 100644
> --- a/drivers/fpga/microchip-spi.c
> +++ b/drivers/fpga/microchip-spi.c
> @@ -115,7 +115,13 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
>  		return -EINVAL;
>  	}
>  
> +	if (count < MPF_HEADER_SIZE_OFFSET + 1)
> +		return -EINVAL;
> +
>  	header_size = *(buf + MPF_HEADER_SIZE_OFFSET);
> +	if (!header_size)
> +		return -EINVAL;
> +
>  	if (header_size > count) {
>  		info->header_size = header_size;
>  		return -EAGAIN;
> @@ -139,6 +145,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 -EAGAIN;
> +
>  		block_id = *(buf + block_id_offset);
>  		block_start = get_unaligned_le32(buf + block_start_offset);
>  
> @@ -183,6 +193,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;

I didn't mention it explicitly, but it kinda follows from the other
comment, do we not just want to ask for a bigger buffer here too?

> +
>  		component_size = get_unaligned_le32(buf +
>  						    components_size_start +
>  						    component_size_byte_num);
> -- 
> 2.43.0
>

Attachment: signature.asc
Description: PGP signature