Re: [PATCH] net: hinic: validate firmware image section bounds

[Simon Horman <horms@kernel.org>]

On Mon, Jul 06, 2026 at 05:34:55PM +0800, Pengpeng Hou wrote:
> The firmware update path copies each section from the image with an offset
> and length supplied by the firmware header. The image validator only
> checked the sum of section lengths and used fw_len + header_size to check
> the file size, but it did not prove that each section offset and length
> fits in the firmware payload.
> 
> Reject images with a truncated header, avoid the fw_len plus header-size
> addition, validate each section type before it is used as a bit index, and
> prove every section range with offset <= fw_len and len <= fw_len - offset.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  .../net/ethernet/huawei/hinic/hinic_devlink.c | 57 ++++++++++++++++---
>  1 file changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> index c977c43e5d5a..0c06fbdadad3 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_devlink.c
> @@ -20,14 +20,25 @@
>  #include "hinic_devlink.h"
>  #include "hinic_hw_dev.h"
>  
> +static bool hinic_fw_section_valid(u32 fw_len, u32 offset, u32 len)
> +{
> +	return offset <= fw_len && len <= fw_len - offset;
> +}
> +
>  static bool check_image_valid(struct hinic_devlink_priv *priv, const u8 *buf,
> -			      u32 image_size, struct host_image_st *host_image)
> +			      size_t image_size, struct host_image_st *host_image)
>  {
> -	struct fw_image_st *fw_image = NULL;
> +	const struct fw_image_st *fw_image;

I don't think the const update is strictly related to this patch,
and thus doesn't belong in this patch.

>  	u32 len = 0;
>  	u32 i;
>  
> -	fw_image = (struct fw_image_st *)buf;
> +	if (image_size < UPDATEFW_IMAGE_HEAD_SIZE) {
> +		dev_err(&priv->hwdev->hwif->pdev->dev,
> +			"Wrong image size read from file\n");
> +		return false;
> +	}
> +
> +	fw_image = (const struct fw_image_st *)buf;
>  
>  	if (fw_image->fw_magic != HINIC_MAGIC_NUM) {
>  		dev_err(&priv->hwdev->hwif->pdev->dev, "Wrong fw_magic read from file, fw_magic: 0x%x\n",
> @@ -41,14 +52,44 @@ static bool check_image_valid(struct hinic_devlink_priv *priv, const u8 *buf,
>  		return false;
>  	}
>  
> +	if (fw_image->fw_len != image_size - UPDATEFW_IMAGE_HEAD_SIZE) {
> +		dev_err(&priv->hwdev->hwif->pdev->dev,
> +			"Wrong data size read from file\n");
> +		return false;
> +	}
> +
>  	for (i = 0; i < fw_image->fw_info.fw_section_cnt; i++) {
> -		len += fw_image->fw_section_info[i].fw_section_len;
> -		host_image->image_section_info[i] = fw_image->fw_section_info[i];
> +		const struct fw_section_info_st *section =
> +			&fw_image->fw_section_info[i];
> +
> +		if (section->fw_section_type >= FILE_TYPE_TOTAL_NUM) {
> +			dev_err(&priv->hwdev->hwif->pdev->dev,
> +				"Wrong section type read from file: %u\n",
> +				section->fw_section_type);
> +			return false;
> +		}
> +
> +		if (!hinic_fw_section_valid(fw_image->fw_len,
> +					    section->fw_section_offset,
> +					    section->fw_section_len)) {
> +			dev_err(&priv->hwdev->hwif->pdev->dev,
> +				"Wrong section size read from file\n");
> +			return false;
> +		}
> +
> +		if (section->fw_section_len > fw_image->fw_len - len) {
> +			dev_err(&priv->hwdev->hwif->pdev->dev,
> +				"Wrong data size read from file\n");
> +			return false;
> +		}
> +
> +		len += section->fw_section_len;
> +		host_image->image_section_info[i] = *section;
>  	}
>  
> -	if (len != fw_image->fw_len ||
> -	    (fw_image->fw_len + UPDATEFW_IMAGE_HEAD_SIZE) != image_size) {
> -		dev_err(&priv->hwdev->hwif->pdev->dev, "Wrong data size read from file\n");
> +	if (len != fw_image->fw_len) {
> +		dev_err(&priv->hwdev->hwif->pdev->dev,
> +			"Wrong data size read from file\n");

Likewise, the update to the dev_err() call, as distinct from the update
to the condition that precedes it, appears to be a whitespace cleanup.
If so, it doesn't belong in this patch.

>  		return false;
>  	}
>  

-- 
pw-bot: changes-requested