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

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

[Pengpeng Hou]

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;
 	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");
 		return false;
 	}
 
-- 
2.43.0

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

[Simon Horman]

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