Re: [PATCH] net: qlcnic: validate unified ROM directory bounds

[Simon Horman]

On Mon, Jul 06, 2026 at 05:36:01PM +0800, Pengpeng Hou wrote:
> The unified ROM parser walks directory and data descriptor tables from the
> firmware file. The existing checks compute table and data limits with
> base + count * size or base + size before comparing with the firmware
> size. Those calculations use fields from the firmware image and can wrap
> before the comparison.
> 
> Add range helpers that validate tables and entries with division and
> subtraction instead of overflowing additions. Pass the firmware size into
> the directory lookup helper, validate each directory entry before reading
> its type field, and validate bootloader, firmware and product-table
> entries before their descriptor fields are used.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
> ---
>  .../net/ethernet/qlogic/qlcnic/qlcnic_init.c  | 135 +++++++++++-------
>  1 file changed, 86 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_init.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_init.c
> index 9192c5ad5a16..56620aed804b 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_init.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_init.c
> @@ -740,21 +740,55 @@ qlcnic_has_mn(struct qlcnic_adapter *adapter)

...

> +static struct uni_table_desc *qlcnic_get_table_desc(const u8 *unirom,
> +						    size_t fw_size, int section)
>  {
> -	u32 i, entries;
>  	struct uni_table_desc *directory = (struct uni_table_desc *) &unirom[0];
> -	entries = le32_to_cpu(directory->num_entries);
> +	u32 entries = le32_to_cpu(directory->num_entries);
> +	u32 entry_size = le32_to_cpu(directory->entry_size);
> +	u32 findex = le32_to_cpu(directory->findex);
> +	u32 i;

I realise that this isn't the existing style of this code, but where
possible please try to move the arrangement of local variable declarations
to follow reverse xmas tree order - longest line to shortest (seems trivial
in this case). If necessary declarations and split should be split (with
all declarations coming first, then a blank line (probably not necessary
here).

>  
> -	for (i = 0; i < entries; i++) {
> +	if (entry_size < QLCNIC_UNI_DIR_ENTRY_MIN_SIZE ||
> +	    !qlcnic_rom_table_valid(fw_size, findex, entries, entry_size))
> +		return NULL;
>  
> -		u32 offs = le32_to_cpu(directory->findex) +
> -			   i * le32_to_cpu(directory->entry_size);
> -		u32 tab_type = le32_to_cpu(*((__le32 *)&unirom[offs] + 8));
> +	for (i = 0; i < entries; i++) {
> +		size_t offs = findex + (size_t)i * entry_size;

I might have declared i as a size_t to avoid this cast.

> +		u32 tab_type = le32_to_cpu(*((__le32 *)&unirom[offs] +
> +						QLCNIC_UNI_DIR_TYPE_OFF));
>  
>  		if (tab_type == section)
> -			return (struct uni_table_desc *) &unirom[offs];
> +			return (struct uni_table_desc *)&unirom[offs];

It seems that the only change above is an update to white space.
Which is a cleanup separate from the intention of this patch.
Please drop such cleanups from this patch as the add noise.

>  	}
>  
>  	return NULL;

...

> @@ -789,31 +822,33 @@ qlcnic_validate_bootld(struct qlcnic_adapter *adapter)
>  {
>  	struct uni_table_desc *tab_desc;
>  	struct uni_data_desc *descr;
> -	u32 offs, tab_size, data_size, idx;
>  	const u8 *unirom = adapter->fw->data;
> +	size_t offs;
> +	u32 data_len, data_off, entry_size, findex, idx;
>  	__le32 temp;
>  
>  	temp = *((__le32 *)&unirom[adapter->file_prd_off] +
>  		 QLCNIC_UNI_BOOTLD_IDX_OFF);
>  	idx = le32_to_cpu(temp);
> -	tab_desc = qlcnic_get_table_desc(unirom, QLCNIC_UNI_DIR_SECT_BOOTLD);
> +	tab_desc = qlcnic_get_table_desc(unirom, adapter->fw->size,
> +					 QLCNIC_UNI_DIR_SECT_BOOTLD);
>  
>  	if (!tab_desc)
>  		return -EINVAL;
>  
> -	tab_size = le32_to_cpu(tab_desc->findex) +
> -		   le32_to_cpu(tab_desc->entry_size) * (idx + 1);
> -
> -	if (adapter->fw->size < tab_size)
> +	entry_size = le32_to_cpu(tab_desc->entry_size);
> +	findex = le32_to_cpu(tab_desc->findex);
> +	if (entry_size < sizeof(*descr) ||
> +	    !qlcnic_rom_entry_valid(adapter->fw->size, findex, entry_size,
> +				     idx))
>  		return -EINVAL;
>  
> -	offs = le32_to_cpu(tab_desc->findex) +
> -	       le32_to_cpu(tab_desc->entry_size) * idx;
> +	offs = findex + (size_t)entry_size * idx;
>  	descr = (struct uni_data_desc *)&unirom[offs];
> +	data_off = le32_to_cpu(descr->findex);
> +	data_len = le32_to_cpu(descr->size);
>  
> -	data_size = le32_to_cpu(descr->findex) + le32_to_cpu(descr->size);
> -
> -	if (adapter->fw->size < data_size)
> +	if (!qlcnic_rom_range_valid(adapter->fw->size, data_off, data_len))
>  		return -EINVAL;

This is one of two issues flagged by AI-generated of this patch at
sashiko.dev that I plan to include in this email. There are other issues,
which you may wish to look into in the context of possible follow-up. But I
feel that these two warrant consideration in the context of this patch as
they are closely related to it's intent.

  This is a pre-existing issue, but does this validation need to enforce a
  minimum length for the bootloader section?
  The firmware loader unconditionally calculates a hardcoded size for the
  bootloader section and iteratively reads this amount:
  qlcnic_load_firmware() {
      ...
      size = (QLCNIC_IMAGE_START - QLCNIC_BOOTLD_START) / 8;
      ...
      for (i = 0; i < size; i++) {
          data = le64_to_cpu(ptr64[i]);
      ...
  }
  If an attacker provides a firmware image with a valid offset near the end
  of the file and a small declared size, couldn't the loop in
  qlcnic_load_firmware() read past the end of the adapter->fw->data buffer?

>  
>  	return 0;
> @@ -825,29 +860,31 @@ qlcnic_validate_fw(struct qlcnic_adapter *adapter)
>  	struct uni_table_desc *tab_desc;
>  	struct uni_data_desc *descr;
>  	const u8 *unirom = adapter->fw->data;
> -	u32 offs, tab_size, data_size, idx;
> +	size_t offs;
> +	u32 data_len, data_off, entry_size, findex, idx;
>  	__le32 temp;
>  
>  	temp = *((__le32 *)&unirom[adapter->file_prd_off] +
>  		 QLCNIC_UNI_FIRMWARE_IDX_OFF);
>  	idx = le32_to_cpu(temp);
> -	tab_desc = qlcnic_get_table_desc(unirom, QLCNIC_UNI_DIR_SECT_FW);
> +	tab_desc = qlcnic_get_table_desc(unirom, adapter->fw->size,
> +					 QLCNIC_UNI_DIR_SECT_FW);
>  
>  	if (!tab_desc)
>  		return -EINVAL;
>  
> -	tab_size = le32_to_cpu(tab_desc->findex) +
> -		   le32_to_cpu(tab_desc->entry_size) * (idx + 1);
> -
> -	if (adapter->fw->size < tab_size)
> +	entry_size = le32_to_cpu(tab_desc->entry_size);
> +	findex = le32_to_cpu(tab_desc->findex);
> +	if (entry_size < sizeof(*descr) ||
> +	    !qlcnic_rom_entry_valid(adapter->fw->size, findex, entry_size,
> +				     idx))
>  		return -EINVAL;
>  
> -	offs = le32_to_cpu(tab_desc->findex) +
> -	       le32_to_cpu(tab_desc->entry_size) * idx;
> +	offs = findex + (size_t)entry_size * idx;
>  	descr = (struct uni_data_desc *)&unirom[offs];
> -	data_size = le32_to_cpu(descr->findex) + le32_to_cpu(descr->size);
> -
> -	if (adapter->fw->size < data_size)
> +	data_off = le32_to_cpu(descr->findex);
> +	data_len = le32_to_cpu(descr->size);
> +	if (!qlcnic_rom_range_valid(adapter->fw->size, data_off, data_len))
>  		return -EINVAL;
>  
>  	return 0;

This is the other portion of the AI-generated review of this patch on
sashiko.dev that I'd like to as you to consider in the context of this patch.

  This is a pre-existing issue, but does this validation need to verify that
  data_len is at least 17 bytes?
  In qlcnic_get_fw_version(), the version string pointer is calculated like
  this:
  qlcnic_get_fw_version() {
      ...
      ver_str = fw->data + le32_to_cpu(fw_data_desc->findex) +
                le32_to_cpu(fw_data_desc->size) - 17;
      ...
  }
  If the firmware image specifies a size less than 17, won't
  le32_to_cpu(fw_data_desc->size) - 17 wrap around due to 32-bit unsigned
  arithmetic, leading to an out-of-bounds pointer well outside the fw->data
  buffer?

...

-- 
pw-bot: changes-requested