[PATCH] netxen: validate unified ROM directory bounds

[PATCH] netxen: validate unified ROM directory bounds

[Pengpeng Hou]

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@iscas.ac.cn>
---
 .../ethernet/qlogic/netxen/netxen_nic_init.c  | 148 +++++++++++-------
 1 file changed, 90 insertions(+), 58 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
index 8bc4e2b69569..5722c55fa0cc 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
@@ -561,20 +561,55 @@ int netxen_pinit_from_rom(struct netxen_adapter *adapter)
 	return 0;
 }
 
-static struct uni_table_desc *nx_get_table_desc(const u8 *unirom, int section)
+#define NX_UNI_DIR_TYPE_OFF		8
+#define NX_UNI_DIR_ENTRY_MIN_SIZE \
+	((NX_UNI_DIR_TYPE_OFF + 1) * sizeof(u32))
+#define NX_UNI_PRODUCT_ENTRY_MIN_SIZE \
+	((NX_UNI_FIRMWARE_IDX_OFF + 1) * sizeof(u32))
+
+static bool netxen_rom_range_valid(size_t size, u32 off, u32 len)
+{
+	return off <= size && len <= size - off;
+}
+
+static bool netxen_rom_table_valid(size_t size, u32 off, u32 n, u32 esz)
+{
+	if (off > size)
+		return false;
+	if (!esz)
+		return n == 0;
+
+	return n <= (size - off) / esz;
+}
+
+static bool netxen_rom_entry_valid(size_t size, u32 off, u32 esz, u32 idx)
+{
+	if (!esz || off > size)
+		return false;
+
+	return idx < (size - off) / esz;
+}
+
+static struct uni_table_desc *
+nx_get_table_desc(const u8 *unirom, size_t fw_size, int section)
 {
-	uint32_t i;
 	struct uni_table_desc *directory = (struct uni_table_desc *) &unirom[0];
-	__le32 entries = cpu_to_le32(directory->num_entries);
+	u32 entries = cpu_to_le32(directory->num_entries);
+	u32 entry_size = cpu_to_le32(directory->entry_size);
+	u32 findex = cpu_to_le32(directory->findex);
+	u32 i;
 
-	for (i = 0; i < entries; i++) {
+	if (entry_size < NX_UNI_DIR_ENTRY_MIN_SIZE ||
+	    !netxen_rom_table_valid(fw_size, findex, entries, entry_size))
+		return NULL;
 
-		__le32 offs = cpu_to_le32(directory->findex) +
-				(i * cpu_to_le32(directory->entry_size));
-		__le32 tab_type = cpu_to_le32(*((u32 *)&unirom[offs] + 8));
+	for (i = 0; i < entries; i++) {
+		size_t offs = findex + (size_t)i * entry_size;
+		u32 tab_type = cpu_to_le32(*((u32 *)&unirom[offs] +
+						NX_UNI_DIR_TYPE_OFF));
 
 		if (tab_type == section)
-			return (struct uni_table_desc *) &unirom[offs];
+			return (struct uni_table_desc *)&unirom[offs];
 	}
 
 	return NULL;
@@ -586,20 +621,18 @@ static int
 netxen_nic_validate_header(struct netxen_adapter *adapter)
 {
 	const u8 *unirom = adapter->fw->data;
-	struct uni_table_desc *directory = (struct uni_table_desc *) &unirom[0];
+	struct uni_table_desc *directory = (struct uni_table_desc *)&unirom[0];
 	u32 fw_file_size = adapter->fw->size;
-	u32 tab_size;
-	__le32 entries;
-	__le32 entry_size;
+	u32 entries, entry_size, findex;
 
 	if (fw_file_size < QLCNIC_FILEHEADER_SIZE)
 		return -EINVAL;
 
 	entries = cpu_to_le32(directory->num_entries);
 	entry_size = cpu_to_le32(directory->entry_size);
-	tab_size = cpu_to_le32(directory->findex) + (entries * entry_size);
+	findex = cpu_to_le32(directory->findex);
 
-	if (fw_file_size < tab_size)
+	if (!netxen_rom_table_valid(fw_file_size, findex, entries, entry_size))
 		return -EINVAL;
 
 	return 0;
@@ -611,30 +644,30 @@ netxen_nic_validate_bootld(struct netxen_adapter *adapter)
 	struct uni_table_desc *tab_desc;
 	struct uni_data_desc *descr;
 	const u8 *unirom = adapter->fw->data;
-	__le32 idx = cpu_to_le32(*((int *)&unirom[adapter->file_prd_off] +
-				NX_UNI_BOOTLD_IDX_OFF));
-	u32 offs;
-	u32 tab_size;
-	u32 data_size;
+	u32 data_len, data_off, entry_size, findex, idx;
+	u32 section = NX_UNI_DIR_SECT_BOOTLD;
+	size_t offs;
 
-	tab_desc = nx_get_table_desc(unirom, NX_UNI_DIR_SECT_BOOTLD);
+	idx = cpu_to_le32(*((int *)&unirom[adapter->file_prd_off] +
+			     NX_UNI_BOOTLD_IDX_OFF));
+	tab_desc = nx_get_table_desc(unirom, adapter->fw->size, section);
 
 	if (!tab_desc)
 		return -EINVAL;
 
-	tab_size = cpu_to_le32(tab_desc->findex) +
-			(cpu_to_le32(tab_desc->entry_size) * (idx + 1));
-
-	if (adapter->fw->size < tab_size)
+	entry_size = cpu_to_le32(tab_desc->entry_size);
+	findex = cpu_to_le32(tab_desc->findex);
+	if (entry_size < sizeof(*descr) ||
+	    !netxen_rom_entry_valid(adapter->fw->size, findex, entry_size,
+				     idx))
 		return -EINVAL;
 
-	offs = cpu_to_le32(tab_desc->findex) +
-		(cpu_to_le32(tab_desc->entry_size) * (idx));
+	offs = findex + (size_t)entry_size * idx;
 	descr = (struct uni_data_desc *)&unirom[offs];
+	data_off = cpu_to_le32(descr->findex);
+	data_len = cpu_to_le32(descr->size);
 
-	data_size = cpu_to_le32(descr->findex) + cpu_to_le32(descr->size);
-
-	if (adapter->fw->size < data_size)
+	if (!netxen_rom_range_valid(adapter->fw->size, data_off, data_len))
 		return -EINVAL;
 
 	return 0;
@@ -646,29 +679,30 @@ netxen_nic_validate_fw(struct netxen_adapter *adapter)
 	struct uni_table_desc *tab_desc;
 	struct uni_data_desc *descr;
 	const u8 *unirom = adapter->fw->data;
-	__le32 idx = cpu_to_le32(*((int *)&unirom[adapter->file_prd_off] +
-				NX_UNI_FIRMWARE_IDX_OFF));
-	u32 offs;
-	u32 tab_size;
-	u32 data_size;
+	u32 data_len, data_off, entry_size, findex, idx;
+	u32 section = NX_UNI_DIR_SECT_FW;
+	size_t offs;
 
-	tab_desc = nx_get_table_desc(unirom, NX_UNI_DIR_SECT_FW);
+	idx = cpu_to_le32(*((int *)&unirom[adapter->file_prd_off] +
+			     NX_UNI_FIRMWARE_IDX_OFF));
+	tab_desc = nx_get_table_desc(unirom, adapter->fw->size, section);
 
 	if (!tab_desc)
 		return -EINVAL;
 
-	tab_size = cpu_to_le32(tab_desc->findex) +
-			(cpu_to_le32(tab_desc->entry_size) * (idx + 1));
-
-	if (adapter->fw->size < tab_size)
+	entry_size = cpu_to_le32(tab_desc->entry_size);
+	findex = cpu_to_le32(tab_desc->findex);
+	if (entry_size < sizeof(*descr) ||
+	    !netxen_rom_entry_valid(adapter->fw->size, findex, entry_size,
+				     idx))
 		return -EINVAL;
 
-	offs = cpu_to_le32(tab_desc->findex) +
-		(cpu_to_le32(tab_desc->entry_size) * (idx));
+	offs = findex + (size_t)entry_size * idx;
 	descr = (struct uni_data_desc *)&unirom[offs];
-	data_size = cpu_to_le32(descr->findex) + cpu_to_le32(descr->size);
+	data_off = cpu_to_le32(descr->findex);
+	data_len = cpu_to_le32(descr->size);
 
-	if (adapter->fw->size < data_size)
+	if (!netxen_rom_range_valid(adapter->fw->size, data_off, data_len))
 		return -EINVAL;
 
 	return 0;
@@ -682,39 +716,37 @@ netxen_nic_validate_product_offs(struct netxen_adapter *adapter)
 	const u8 *unirom = adapter->fw->data;
 	int mn_present = (NX_IS_REVISION_P2(adapter->ahw.revision_id)) ?
 			1 : netxen_p3_has_mn(adapter);
-	__le32 entries;
-	__le32 entry_size;
-	u32 tab_size;
-	u32 i;
+	u32 entries, entry_size, findex, i;
+	u32 section = NX_UNI_DIR_SECT_PRODUCT_TBL;
 
-	ptab_descr = nx_get_table_desc(unirom, NX_UNI_DIR_SECT_PRODUCT_TBL);
+	ptab_descr = nx_get_table_desc(unirom, adapter->fw->size, section);
 	if (ptab_descr == NULL)
 		return -EINVAL;
 
 	entries = cpu_to_le32(ptab_descr->num_entries);
 	entry_size = cpu_to_le32(ptab_descr->entry_size);
-	tab_size = cpu_to_le32(ptab_descr->findex) + (entries * entry_size);
-
-	if (adapter->fw->size < tab_size)
+	findex = cpu_to_le32(ptab_descr->findex);
+	if (entry_size < NX_UNI_PRODUCT_ENTRY_MIN_SIZE ||
+	    !netxen_rom_table_valid(adapter->fw->size, findex, entries,
+				     entry_size))
 		return -EINVAL;
 
 nomn:
 	for (i = 0; i < entries; i++) {
-
-		__le32 flags, file_chiprev, offs;
+		size_t offs;
+		__le32 flags, file_chiprev;
 		u8 chiprev = adapter->ahw.revision_id;
 		uint32_t flagbit;
 
-		offs = cpu_to_le32(ptab_descr->findex) +
-				(i * cpu_to_le32(ptab_descr->entry_size));
+		offs = findex + (size_t)i * entry_size;
 		flags = cpu_to_le32(*((int *)&unirom[offs] + NX_UNI_FLAGS_OFF));
 		file_chiprev = cpu_to_le32(*((int *)&unirom[offs] +
-							NX_UNI_CHIP_REV_OFF));
+						    NX_UNI_CHIP_REV_OFF));
 
 		flagbit = mn_present ? 1 : 2;
 
 		if ((chiprev == file_chiprev) &&
-					((1ULL << flagbit) & flags)) {
+		    ((1ULL << flagbit) & flags)) {
 			adapter->file_prd_off = offs;
 			return 0;
 		}
@@ -767,7 +799,7 @@ static struct uni_data_desc *nx_get_data_desc(struct netxen_adapter *adapter,
 	struct uni_table_desc *tab_desc;
 	__le32 offs;
 
-	tab_desc = nx_get_table_desc(unirom, section);
+	tab_desc = nx_get_table_desc(unirom, adapter->fw->size, section);
 
 	if (tab_desc == NULL)
 		return NULL;
-- 
2.43.0

Re: [PATCH] netxen: validate unified ROM directory bounds

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
Subject: netxen: validate unified ROM directory bounds

This patch updates the unified ROM parser for the netxen driver to use safer
range validation helpers. It calculates bounds using division and subtraction
to avoid integer overflow vulnerabilities when validating directory entries,
bootloader, firmware, and product tables.

> diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> index 8bc4e2b69569e..5722c55fa0cce 100644
> --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
[ ... ]
> @@ -561,20 +561,55 @@ int netxen_pinit_from_rom(struct netxen_adapter *adapter)
[ ... ]
> +static struct uni_table_desc *
> +nx_get_table_desc(const u8 *unirom, size_t fw_size, int section)
>  {
>  	struct uni_table_desc *directory = (struct uni_table_desc *) &unirom[0];
> -	__le32 entries = cpu_to_le32(directory->num_entries);
> +	u32 entries = cpu_to_le32(directory->num_entries);
> +	u32 entry_size = cpu_to_le32(directory->entry_size);
> +	u32 findex = cpu_to_le32(directory->findex);
> +	u32 i;
>  
> -	for (i = 0; i < entries; i++) {
> +	if (entry_size < NX_UNI_DIR_ENTRY_MIN_SIZE ||
> +	    !netxen_rom_table_valid(fw_size, findex, entries, entry_size))
> +		return NULL;
>  
> -		__le32 offs = cpu_to_le32(directory->findex) +
> -				(i * cpu_to_le32(directory->entry_size));
> -		__le32 tab_type = cpu_to_le32(*((u32 *)&unirom[offs] + 8));
> +	for (i = 0; i < entries; i++) {
> +		size_t offs = findex + (size_t)i * entry_size;
> +		u32 tab_type = cpu_to_le32(*((u32 *)&unirom[offs] +
> +						NX_UNI_DIR_TYPE_OFF));

[Severity: High]
This isn't a bug introduced by this patch, but can this cast trigger an
unaligned access panic?

If findex or entry_size provided by the firmware header are unaligned,
offs will not be a multiple of 4. Casting &unirom[offs] directly to a
32-bit integer pointer and dereferencing it could fault on architectures that
do not support unaligned memory accesses natively.

>  
>  		if (tab_type == section)
> -			return (struct uni_table_desc *) &unirom[offs];
> +			return (struct uni_table_desc *)&unirom[offs];
>  	}
>  
>  	return NULL;
[ ... ]
> @@ -611,30 +644,30 @@ netxen_nic_validate_bootld(struct netxen_adapter *adapter)
[ ... ]
> -	offs = cpu_to_le32(tab_desc->findex) +
> -		(cpu_to_le32(tab_desc->entry_size) * (idx));
> +	offs = findex + (size_t)entry_size * idx;
>  	descr = (struct uni_data_desc *)&unirom[offs];
> +	data_off = cpu_to_le32(descr->findex);
> +	data_len = cpu_to_le32(descr->size);
>  
> -	data_size = cpu_to_le32(descr->findex) + cpu_to_le32(descr->size);
> -
> -	if (adapter->fw->size < data_size)
> +	if (!netxen_rom_range_valid(adapter->fw->size, data_off, data_len))
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does netxen_load_firmware() ignore this
validated data_len?

It appears netxen_load_firmware() uses a hardcoded copy size:

netxen_load_firmware() {
    ...
    size = (NETXEN_IMAGE_START - NETXEN_BOOTLD_START) / 8;
    ptr64 = (u64 *)nx_get_bootld_offs(adapter);
    for (i = 0; i < size; i++) {
        data = cpu_to_le64(ptr64[i]);
    ...
}

If data_len is small, couldn't this hardcoded size cause an out-of-bounds slab
memory read past the end of the firmware file data?

>  
>  	return 0;
> @@ -646,29 +679,30 @@ netxen_nic_validate_fw(struct netxen_adapter *adapter)
[ ... ]
> -	offs = cpu_to_le32(tab_desc->findex) +
> -		(cpu_to_le32(tab_desc->entry_size) * (idx));
> +	offs = findex + (size_t)entry_size * idx;
>  	descr = (struct uni_data_desc *)&unirom[offs];
> -	data_size = cpu_to_le32(descr->findex) + cpu_to_le32(descr->size);
> +	data_off = cpu_to_le32(descr->findex);
> +	data_len = cpu_to_le32(descr->size);
>  
> -	if (adapter->fw->size < data_size)
> +	if (!netxen_rom_range_valid(adapter->fw->size, data_off, data_len))
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this validation need to enforce a
minimum length for data_len (size)?

In nx_get_fw_version(), the version string pointer calculation relies on size:

nx_get_fw_version() {
    ...
    ver_str = fw->data + cpu_to_le32(fw_data_desc->findex) +
              cpu_to_le32(fw_data_desc->size) - 17;
    for (i = 0; i < 12; i++) {
        if (!strncmp(&ver_str[i], "REV=", 4)) {
    ...
}

If a malformed firmware image provides an entry where size is less than 17,
wouldn't the subtraction wrap around and result in a massive out-of-bounds
memory read?

Additionally, in netxen_load_firmware(), if the firmware data_len is not a
multiple of 8, it seems the remainder is handled unconditionally by reading a
full 8-byte word:

netxen_load_firmware() {
    ...
    size = (__force u32)nx_get_fw_size(adapter) % 8;
    if (size) {
        data = cpu_to_le64(ptr64[i]);
    ...
}

If fw->size is exactly data_off + data_len, wouldn't reading an 8-byte chunk
at an unaligned remainder boundary access up to 7 bytes past the end of the
fw->data buffer?

>  
>  	return 0;
> @@ -682,39 +716,37 @@ netxen_nic_validate_product_offs(struct netxen_adapter *adapter)
[ ... ]
>  nomn:
>  	for (i = 0; i < entries; i++) {
> -
> -		__le32 flags, file_chiprev, offs;
> +		size_t offs;
> +		__le32 flags, file_chiprev;
>  		u8 chiprev = adapter->ahw.revision_id;
>  		uint32_t flagbit;
>  
> -		offs = cpu_to_le32(ptab_descr->findex) +
> -				(i * cpu_to_le32(ptab_descr->entry_size));
> +		offs = findex + (size_t)i * entry_size;
>  		flags = cpu_to_le32(*((int *)&unirom[offs] + NX_UNI_FLAGS_OFF));

[Severity: High]
This isn't a bug introduced by this patch, but can this cast to (int *) also
trigger an unaligned access panic if the firmware supplies an unaligned
findex or entry_size?

>  		file_chiprev = cpu_to_le32(*((int *)&unirom[offs] +
> -							NX_UNI_CHIP_REV_OFF));
> +						    NX_UNI_CHIP_REV_OFF));
>  
>  		flagbit = mn_present ? 1 : 2;
>
-- 
pw-bot: changes-requested