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

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

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;
>