[PATCH] ixgbe: fix incomplete ACI command buffer backup on retry

[PATCH] ixgbe: fix incomplete ACI command buffer backup on retry

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

ixgbe_aci_send_cmd() backs up the indirect command buffer so that it can
be restored before an EBUSY retry, but the backup only copies the very
first byte of it:

    buf_cpy = kmalloc(buf_size, GFP_KERNEL);
    ...
    *buf_cpy = *(u8 *)buf;

while the restore path copies the full buffer back out of it:

    if (buf)
        memcpy(buf, buf_cpy, buf_size);

So every retry hands the firmware a buffer whose content, apart from the
first byte, is uninitialized slab memory, and on a read command the
caller's buffer is overwritten with that garbage when the retry finally
gives up. The buffer of a read command is an output buffer, so the caller
does not initialize it either.

Use kmemdup() to copy the whole buffer, exactly like the equivalent
ice_sq_send_cmd() already does.

Also skip the backup when buf_size is 0. kmalloc(0) returns
ZERO_SIZE_PTR and the single byte store above would write to it, and
restore based on buf_cpy instead of buf so that this case never passes
ZERO_SIZE_PTR to memcpy().

Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
index 4d8ae5b56145..5dd88ee7ea58 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
@@ -214,11 +214,10 @@ int ixgbe_aci_send_cmd(struct ixgbe_hw *hw, struct libie_aq_desc *desc,
 
 	is_cmd_for_retry = ixgbe_should_retry_aci_send_cmd_execute(opcode);
 	if (is_cmd_for_retry) {
-		if (buf) {
-			buf_cpy = kmalloc(buf_size, GFP_KERNEL);
+		if (buf && buf_size) {
+			buf_cpy = kmemdup(buf, buf_size, GFP_KERNEL);
 			if (!buf_cpy)
 				return -ENOMEM;
-			*buf_cpy = *(u8 *)buf;
 		}
 		desc_cpy = *desc;
 	}
@@ -234,7 +233,7 @@ int ixgbe_aci_send_cmd(struct ixgbe_hw *hw, struct libie_aq_desc *desc,
 		    last_status != LIBIE_AQ_RC_EBUSY)
 			break;
 
-		if (buf)
+		if (buf_cpy)
 			memcpy(buf, buf_cpy, buf_size);
 		*desc = desc_cpy;
 
-- 
2.25.1

Re: [PATCH] ixgbe: fix incomplete ACI command buffer backup on retry

[Przemek Kitszel]

On 9/12/26 03:34, Linkui Xiao wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> ixgbe_aci_send_cmd() backs up the indirect command buffer so that it can
> be restored before an EBUSY retry, but the backup only copies the very
> first byte of it:
> 
>      buf_cpy = kmalloc(buf_size, GFP_KERNEL);
>      ...
>      *buf_cpy = *(u8 *)buf;
> 
> while the restore path copies the full buffer back out of it:
> 
>      if (buf)
>          memcpy(buf, buf_cpy, buf_size);
> 
> So every retry hands the firmware a buffer whose content, apart from the
> first byte, is uninitialized slab memory, and on a read command the
> caller's buffer is overwritten with that garbage when the retry finally
> gives up. The buffer of a read command is an output buffer, so the caller
> does not initialize it either.
> 
> Use kmemdup() to copy the whole buffer, exactly like the equivalent
> ice_sq_send_cmd() already does.
> 
> Also skip the backup when buf_size is 0. kmalloc(0) returns
> ZERO_SIZE_PTR and the single byte store above would write to it, and
> restore based on buf_cpy instead of buf so that this case never passes
> ZERO_SIZE_PTR to memcpy().
> 
> Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
>   drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> index 4d8ae5b56145..5dd88ee7ea58 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> @@ -214,11 +214,10 @@ int ixgbe_aci_send_cmd(struct ixgbe_hw *hw, struct libie_aq_desc *desc,
>   
>   	is_cmd_for_retry = ixgbe_should_retry_aci_send_cmd_execute(opcode);
>   	if (is_cmd_for_retry) {
> -		if (buf) {
> -			buf_cpy = kmalloc(buf_size, GFP_KERNEL);
> +		if (buf && buf_size) {
> +			buf_cpy = kmemdup(buf, buf_size, GFP_KERNEL);
>   			if (!buf_cpy)
>   				return -ENOMEM;
> -			*buf_cpy = *(u8 *)buf;
>   		}
>   		desc_cpy = *desc;
>   	}
> @@ -234,7 +233,7 @@ int ixgbe_aci_send_cmd(struct ixgbe_hw *hw, struct libie_aq_desc *desc,
>   		    last_status != LIBIE_AQ_RC_EBUSY)
>   			break;
>   
> -		if (buf)
> +		if (buf_cpy)

ough ;F, thank your for the cleanup
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

>   			memcpy(buf, buf_cpy, buf_size);
>   		*desc = desc_cpy;
>   

Re: [PATCH] ixgbe: fix incomplete ACI command buffer backup on retry

[netdev-bot+sashiko]

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

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

- [Medium] Commit message overstates the impact of the fixed bug.
- [Low] Commit message names the wrong ice function as the reference
  implementation.

--

ixgbe: fix incomplete ACI command buffer backup on retry

The patch replaces the kmalloc() plus single-byte store in
ixgbe_aci_send_cmd() with kmemdup() so the whole indirect command buffer
is saved before an EBUSY retry, skips the backup when buf_size is 0, and
keys the restore off buf_cpy instead of buf. The code change itself looks
correct; the questions below are about the changelog only, so the diff
hunks are snipped.

> So every retry hands the firmware a buffer whose content, apart from the
> first byte, is uninitialized slab memory, and on a read command the
> caller's buffer is overwritten with that garbage when the retry finally
> gives up.

[Severity: Medium]
Can the firmware actually see that garbage?  In
ixgbe_aci_send_cmd_execute() the buffer only reaches the HIBA registers
when the descriptor carries LIBIE_AQ_FLAG_RD:

drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c:ixgbe_aci_send_cmd_execute() {
	...
	if (desc->flags & cpu_to_le16(LIBIE_AQ_FLAG_RD)) {
		for (i = 0; i < buf_size / 4; i++)
			IXGBE_WRITE_REG(hw, IXGBE_PF_HIBA(i), ((u32 *)buf)[i]);
	...
}

The backup/restore path only runs for the opcodes listed in
ixgbe_should_retry_aci_send_cmd_execute(): disable_rxen, get_phy_caps,
get_link_status and get_link_topo.  disable_rxen and get_link_topo pass
(NULL, 0), and the two that do pass a buffer (ixgbe_aci_get_phy_caps()
and ixgbe_aci_get_link_info()) build their descriptor with
ixgbe_fill_dflt_direct_cmd_desc(), which sets only LIBIE_AQ_FLAG_SI.
LIBIE_AQ_FLAG_RD is OR-ed in only by set_phy_cfg, the NVM write,
nvm_pkg_data and nvm_pass_component_tbl paths, none of which are
retryable.

Also, on both EBUSY exits ixgbe_aci_send_cmd_execute() returns before the
"Write a response values to a buf" block, so buf is not touched by an
EBUSY attempt and the restore is effectively a no-op there.

Would it be more accurate to describe the effect as the caller's output
buffer being filled with uninitialized slab bytes once retries are
exhausted, rather than as garbage being handed to the firmware on every
retry?  As written the message suggests a functional firmware
interaction problem, which affects how this reads for stable triage.

> Use kmemdup() to copy the whole buffer, exactly like the equivalent
> ice_sq_send_cmd() already does.

[Severity: Low]
Is ice_sq_send_cmd() the right reference here?  That function is the
low-level control queue submit routine, the counterpart of
ixgbe_aci_send_cmd_execute(), and it has no buffer backup.  The
kmemdup()-based backup and restore-on-EBUSY logic lives in
ice_sq_send_cmd_retry() in drivers/net/ethernet/intel/ice/ice_common.c:

drivers/net/ethernet/intel/ice/ice_common.c:ice_sq_send_cmd_retry() {
	...
	if (is_cmd_for_retry) {
		if (buf) {
			buf_cpy = kmemdup(buf, buf_size, GFP_KERNEL);
			if (!buf_cpy)
				return -ENOMEM;
		}
	...
		if (buf_cpy)
			memcpy(buf, buf_cpy, buf_size);
	...
}

Could the changelog name ice_sq_send_cmd_retry() instead?

[ ... ]

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