Re: [Intel-wired-lan] [PATCH net v2] i40e: limit the DDP profile count returned by the firmware

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Incomplete fix in i40e_ddp_does_profile_exist() and
  i40e_ddp_does_profile_overlap()…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but it is the
  direct enabler of the residual garbage-compare above and would be…

--

i40e: limit the DDP profile count returned by the firmware

The patch clamps the firmware reported profile count in
i40e_ddp_does_profile_exist() and i40e_ddp_does_profile_overlap() to
I40E_MAX_PROFILE_NUM so the loops cannot index past the on-stack
buff[] array, and switches the loop counter to unsigned.  The clamp
value is the compile time capacity of buff[].

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ddp.c b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
> index daa9f2c42f70..26dccd4236c1 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ddp.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
> @@ -54,8 +54,8 @@ static int i40e_ddp_does_profile_exist(struct i40e_hw *hw,
>  {
>  	struct i40e_ddp_profile_list *profile_list;
>  	u8 buff[I40E_PROFILE_LIST_SIZE];
                                        ^^^^

[Severity: Medium]
This isn't something introduced by this patch, but since both hunks touch
these declarations it may be worth handling here: buff[] is 772 bytes of
uninitialized stack in both i40e_ddp_does_profile_exist() and
i40e_ddp_does_profile_overlap(), and it is handed to
i40e_aq_get_ddp_list() as an indirect admin queue buffer.

i40e_asq_send_command_atomic_exec() does not distinguish input from output
buffers:

	/* if buff is not NULL assume indirect command */
	if (buff != NULL) {
		dma_buff = &(hw->aq.asq.r.asq_bi[hw->aq.asq.next_to_use]);
		/* copy the user buff into the respective DMA buff */
		memcpy(dma_buff->va, buff, buff_size);

Does this publish 772 bytes of raw kernel stack into DMA memory that the
NIC firmware can read?  On completion the same full buff_size is copied
back:

		if (buff != NULL)
			memcpy(buff, dma_buff->va, buff_size);

so any byte the firmware did not overwrite still holds the old stack
image that the clamped loops then compare against.  Would declaring the
buffer as

	u8 buff[I40E_PROFILE_LIST_SIZE] = {};

in both helpers remove both the device visible stack content and the
comparisons against never written bytes?

>  	int status;
> -	int i;
>  
>  	status = i40e_aq_get_ddp_list(hw, buff, I40E_PROFILE_LIST_SIZE, 0,
>  				      NULL);
> @@ -63,7 +63,12 @@ static int i40e_ddp_does_profile_exist(struct i40e_hw *hw,
>  		return -1;
>  
>  	profile_list = (struct i40e_ddp_profile_list *)buff;
> -	for (i = 0; i < profile_list->p_count; i++) {
> +	/* Never walk past the end of buff[], the profile count reported by
> +	 * the firmware is not guaranteed to fit into the buffer we gave it.
> +	 */
> +	p_count = min_t(u32, profile_list->p_count, I40E_MAX_PROFILE_NUM);
> +
> +	for (i = 0; i < p_count; i++) {
>  		if (i40e_ddp_profiles_eq(pinfo, &profile_list->p_info[i]))
>  			return 1;
>  	}

[Severity: Medium]
The arithmetic behind the clamp checks out: sizeof(struct
i40e_profile_info) is 48 and I40E_PROFILE_LIST_SIZE is 48 * 16 + 4, so
p_info[15] ends exactly at the last byte of buff[] and the out of bounds
walk described in the commit message is closed.

The remaining question is that the new bound is the static capacity of
buff[] rather than the extent of data the firmware actually returned.

i40e_aq_get_ddp_list() sets desc.datalen on input but never reports the
response length back to the caller, and
i40e_asq_send_command_atomic_exec() copies the full requested buff_size
back from the reused DMA bounce buffer without consulting the response
descriptor.

So if the firmware reports a p_count larger than the number of records it
actually wrote, but still no larger than 16, do these loops compare
against p_info[] slots that were never filled?  In
i40e_ddp_does_profile_overlap() below, i40e_ddp_profiles_overlap() treats
a zeroed or stale entry as a conflict:

	unsigned int group_id_old = FIELD_GET(0x00FF0000, old->track_id);
	unsigned int group_id_new = FIELD_GET(0x00FF0000, new->track_id);

	/* 0x00 group must be only the first */
	if (group_id_new == 0)
		return true;

which i40e_ddp_load() turns into a hard rejection of a legitimate load:

	if (istatus > 0) {
		netdev_err(netdev, "DDP profile overlaps with existing one.");
		return -EINVAL;
	}

The equality helper can similarly produce a "DDP profile already loaded."
verdict from stale bytes.

The other direction is the p_count > 16 case that the commit message
targets.  Both helpers now silently truncate the scan and return 0, and
i40e_ddp_load() consumes that as a definitive answer: an add proceeds to
i40e_write_profile()/i40e_add_pinfo() without having examined every
reported profile, and the is_add == false path rejects a profile that may
in fact be loaded:

	} else {
		if (istatus == 0) {
			netdev_err(netdev,
				   "DDP profile for deletion does not exist.");
			return -EINVAL;
		}
	}

Would it be better to derive the iteration bound from the length the
firmware actually returned, by plumbing the response datalen out of
i40e_aq_get_ddp_list(), and to return an error instead of silently
clamping when p_count exceeds I40E_MAX_PROFILE_NUM?

The reachable path here is ethtool -f, so it needs CAP_NET_ADMIN plus
firmware reporting a count that does not match what it wrote, which is
the same untrusted firmware assumption the patch already adopts.

> @@ -109,8 +114,8 @@ static int i40e_ddp_does_profile_overlap(struct i40e_hw *hw,
>  {
>  	struct i40e_ddp_profile_list *profile_list;
>  	u8 buff[I40E_PROFILE_LIST_SIZE];
> +	u32 i, p_count;
>  	int status;
> -	int i;
>  
>  	status = i40e_aq_get_ddp_list(hw, buff, I40E_PROFILE_LIST_SIZE, 0,
>  				      NULL);
> @@ -118,7 +123,12 @@ static int i40e_ddp_does_profile_overlap(struct i40e_hw *hw,
>  		return -EIO;
>  
>  	profile_list = (struct i40e_ddp_profile_list *)buff;
> -	for (i = 0; i < profile_list->p_count; i++) {
> +	/* Never walk past the end of buff[], the profile count reported by
> +	 * the firmware is not guaranteed to fit into the buffer we gave it.
> +	 */
> +	p_count = min_t(u32, profile_list->p_count, I40E_MAX_PROFILE_NUM);
> +
> +	for (i = 0; i < p_count; i++) {
>  		if (i40e_ddp_profiles_overlap(pinfo,
>  					      &profile_list->p_info[i]))
>  			return 1;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260920063244.1927792-1-xiaolinkui%40126.com