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

[Xu Yilun]

> @@ -139,6 +145,12 @@ 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) {
> +			info->header_size = block_start_offset + sizeof(u32);
> +			return -EAGAIN;
> +		}
> +

The image header has already been extended up to all look-up table for
blocks, is it?

	header_size += blocks_num * MPF_LOOKUP_TABLE_RECORD_SIZE;
	if (header_size > count) {
		info->header_size = header_size;
		return -EAGAIN;
	}

>  		block_id = *(buf + block_id_offset);
>  		block_start = get_unaligned_le32(buf + block_start_offset);
>  
> @@ -175,6 +187,9 @@ static int mpf_ops_parse_header(struct fpga_manager *mgr,
>  	 * to each other. Image header should be extended by now up to where
>  	 * actual bitstream starts, so no need for overflow check anymore.
>  	 */
> +	if (MPF_DATA_SIZE_OFFSET + sizeof(u16) > count)
> +		return -EINVAL;
> +

Do you notice the comments above? IIUC it says all these header info
should be before actual bitstream starts, if we could ensure this, we
don't need to check the addresses inside the header byte by byte.

I think it is important we understand the structure of the image file
first then meaningfully check the boundaries chunk by chunk, rather than
byte by byte, which makes code unreadable.

Thanks,
Yilun

>  	components_num = get_unaligned_le16(buf + MPF_DATA_SIZE_OFFSET);
>  
>  	for (i = 0; i < components_num; i++) {
> @@ -183,6 +198,11 @@ 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 < components_size_start ||
> +		    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
>