drivers/net/ethernet/intel/i40e/i40e_ddp.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
From: Linkui Xiao <xiaolinkui@kylinos.cn>
i40e_aq_get_ddp_list() writes into a I40E_PROFILE_LIST_SIZE buffer,
which is sized for I40E_MAX_PROFILE_NUM (16) i40e_profile_info entries
plus the 4 byte p_count header. i40e_ddp_does_profile_exist() and
i40e_ddp_does_profile_overlap() then loop over profile_list->p_count
without bounding it, so a firmware reporting more than 16 profiles makes
both helpers walk past the end of the on-stack buff[] and compare
against whatever happens to follow it on the stack.
Clamp the count to the number of entries the buffer can actually hold
and make the loop counter unsigned to match the field type.
Fixes: cdc594e00370 ("i40e: Implement DDP support in i40e driver")
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
v2:
- Reordered variable declarations per Reverse Christmas Tree convention.
- Added Reviewed-by tag from Aleksandr Loktionov.
v1: https://lore.kernel.org/all/20260915120354.610499-1-xiaolinkui@126.com/
drivers/net/ethernet/intel/i40e/i40e_ddp.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
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];
+ u32 i, p_count;
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;
}
@@ -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;
--
2.25.1Thank 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
Thanks for the review. Both findings are addressed in v3. - [Medium] Incomplete fix in i40e_ddp_does_profile_exist() and i40e_ddp_does_profile_overlap() Agreed on the clamping half. Silently limiting the count and then returning 0 hands i40e_ddp_load() a definite answer derived from a list that was only partially read: the add path proceeds without having examined every profile the firmware reported, and the is_add == false path reports a profile as missing that may in fact be loaded. Both helpers already have an error return that i40e_ddp_load() turns into "Failed to fetch loaded profiles." and aborts the operation, so v3 rejects the list instead: if the firmware reports more profiles than I40E_PROFILE_LIST_SIZE can hold, the helpers return -EIO and no scan happens at all. I did not plumb the response datalen out of i40e_aq_get_ddp_list(). For this command the driver has to set desc.datalen to the buffer size it passes in, and i40e_aq_get_ddp_list() does not report the writeback descriptor back to its caller, so the length would have to come either from a new output parameter on that helper (declared in i40e_prototype.h) or from a wb_desc routed through cmd_details. Either way it adds a second firmware supplied number to validate on top of the one this patch is about, and if the firmware leaves the field at the requested 772 bytes the derived bound is exactly the bound we have today. That is a bigger change to the common AQ path than a net fix should carry, so I would rather bound the scan by the size of the buffer the driver owns. The remaining half - comparing against p_info[] slots the firmware never filled - cannot be detected independently of the count, because the driver is not told how many records were actually written. If the firmware reports a count of 16 or less while writing fewer records, the unread slots are indistinguishable from real entries. Zero initialization makes the outcome deterministic (an unwritten entry is all zeroes rather than stale stack) but does not make it correct; deriving the bound from the response length would be needed for that, and that is the larger helper change described above. - [Medium, pre-existing] uninitialized buff[] handed to i40e_aq_get_ddp_list() Agreed, and fixed as suggested: buff[] is now zero initialized in both helpers. i40e_asq_send_command_atomic_exec() copies the full buff_size into the DMA bounce buffer and copies the full buff_size back on completion, so those 772 bytes of stack were both readable by the firmware and compared against afterwards. Both hunks touch these declarations and the root commit is the same (cdc594e00370), so it is handled here rather than as a separate patch; I can split it out if you would rather have it on its own. v3 keeps the unsigned loop counter from v2, and drops the Reviewed-by tag, as the code changed after the review. pw-bot: cr
© 2016 - 2026 Red Hat, Inc.