Re: [net PATCH 01/13] fbnic: Reject a short MAC address attribute from firmware

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch changes fbnic_tlv_attr_addr_copy() to return int:
  -EINVAL when the attribute is not exactly ETH_ALEN long, and in that…

Pre-existing issues:
- [High] fbnic_fw_parse_qsfp_read_resp() in
  drivers/net/ethernet/meta/fbnic/fbnic_fw.c has the same short-BINARY-…
- [Medium] (Baseline decision: not reported.

--

fbnic: Reject a short MAC address attribute from firmware

This patch makes fbnic_tlv_attr_addr_copy() reject a MAC address attribute
that is not exactly ETH_ALEN long and return -EINVAL. It also changes
fbnic_fw_parse_bmc_addrs() to stage the parsed addresses in a local array.
They are copied out only once all of them have parsed.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 49b8813a35643..bcbeb0814b827 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> @@ -609,13 +610,16 @@ static int fbnic_fw_parse_bmc_addrs(u8 bmc_mac_addr[][ETH_ALEN],
>  	if (err)
>  		return err;
>  
> -	/* Copy results into MAC addr array */
> -	for (i = 0; i < len && mac_results[i]; i++)
> -		fbnic_tlv_attr_addr_copy(bmc_mac_addr[i], mac_results[i]);
> +	/* Stage the addresses so a bad one leaves the old set untouched.
> +	 * Unused entries stay zeroed.
> +	 */
> +	for (i = 0; i < len && mac_results[i]; i++) {
> +		err = fbnic_tlv_attr_addr_copy(mac_addr[i], mac_results[i]);
> +		if (err)
> +			return err;
> +	}
>  
> -	/* Zero remaining unused addresses */
> -	while (i < len)
> -		eth_zero_addr(bmc_mac_addr[i++]);
> +	memcpy(bmc_mac_addr, mac_addr, len * ETH_ALEN);

[Severity: Medium]
This is a pre-existing issue, but is anything serializing this write
against the reader of fbd->fw_cap.bmc_mac_addr?

This memcpy() runs from the threaded FW mailbox IRQ path and takes no
lock. fbnic_bmc_rpc_init() walks the same array from the service task
while holding only netdev_lock, which the writer never takes.

The TCAM_REINIT bit handshake in fbnic_bmc_rpc_check() orders the flag
against the data. It does not provide mutual exclusion, though.

Suppose a second CAP_RESP is processed while fbnic_bmc_rpc_init() is
partway through its loop. Can the reader then see a mix of old and new
addresses and program a bogus MACDA TCAM entry?

The baseline had the same race, because it wrote the live array one entry
at a time.

This looks resolved later in the series by "fbnic: Keep the BMC MAC
address set whole across a capabilities update". That commit publishes the
array under write_seqcount_begin()/write_seqcount_end(), and
fbnic_bmc_rpc_init() takes a snapshot in a read_seqcount_retry() loop.

>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c b/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c
> index 3ef3935c0419f..e1ef134ddb944 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c
> @@ -724,12 +724,25 @@ struct fbnic_tlv_msg *fbnic_tlv_test_create(struct fbnic_dev *fbd)
>  	return NULL;
>  }
>  
> -void fbnic_tlv_attr_addr_copy(u8 *dest, struct fbnic_tlv_msg *src)

[ ... ]

> +int fbnic_tlv_attr_addr_copy(u8 *dest, struct fbnic_tlv_msg *src)
>  {
> -	u8 *mac_addr;
> +	if (le16_to_cpu(src->hdr.len) != sizeof(*src) + ETH_ALEN)
> +		return -EINVAL;

[Severity: Low]
Should the two callers in fbnic_tlv_parser_test_attr() also handle the
new return value? They still treat the function as void:

fbnic_tlv_parser_test_attr() {
    ...
	attr = results[FBNIC_TLV_TEST_MSG_MAC_ADDR];
	if (attr)
		fbnic_tlv_attr_addr_copy(str->test_mac, attr);
    ...
			for (i = 0; i < 4 && array_results[i]; i++)
				fbnic_tlv_attr_addr_copy(str->test_mac_array[i],
							 array_results[i]);
    ...
}

If a short MAC attribute comes back in the self-test response, the
destination keeps the zeros from the memset() in fbnic_tlv_parser_test().
The failure then shows up only as a generic memcmp() mismatch, not as
-EINVAL.

If the random expected MAC happens to be all zeros, the test would pass
even though the parse failed.

The prototype is not marked __must_check, so the build won't flag these
callers.

> +
> +	memcpy(dest, fbnic_tlv_attr_get_value_ptr(src), ETH_ALEN);

[Severity: High]
This isn't a bug introduced by this patch, but
fbnic_fw_parse_qsfp_read_resp() in fbnic_fw.c seems to have the same
over-read on a short BINARY attribute that this patch fixes for MAC
addresses. Could it get a similar check?

FBNIC_FW_QSFP_DATA is declared with FBNIC_TLV_ATTR_RAW_DATA. As a result,
fbnic_tlv_attr_validate() accepts any payload from 1 up to TLV_MAX_DATA
bytes. The handler only checks length against the host request:

fbnic_fw_parse_qsfp_read_resp() {
    ...
	if (length != cmpl_data->u.qsfp.length ||
	    offset != cmpl_data->u.qsfp.offset) {
    ...
	data = fbnic_tlv_attr_get_value_ptr(data_hdr);
	memcpy(cmpl_data->u.qsfp.data, data, length);
    ...
}

Say firmware reports a length of 128 but sends a truncated data
attribute. Can this read up to 127 bytes past the end of the attribute
and of the message?

The mailbox Rx pages come from __get_free_page(GFP_KERNEL) in
fbnic_mbx_alloc_rx_msgs() and are not zeroed. If the attribute is near
the end of the page, the read could also run into the next page.

fbnic_get_module_eeprom_by_page() then copies those bytes into the
ethtool buffer that goes back to userspace:

	memcpy(page_data->data, fw_cmpl->u.qsfp.data, page_data->length);

The coredump handler already has the equivalent check:

fbnic_fw_parse_coredump_resp() {
    ...
	if (length > le16_to_cpu(data_hdr->hdr.len) - sizeof(u32)) {
    ...
}

RAW_DATA is variable length, so a check in the validator wouldn't cover
this case. The QSFP handler would need its own payload length check.

>  
> -	mac_addr = fbnic_tlv_attr_get_value_ptr(src);
> -	memcpy(dest, mac_addr, ETH_ALEN);
> +	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179322311834.40667.5383039334126572422.stgit%40ahduyck-xeon-server.home.arpa