Re: [Intel-wired-lan] [PATCH net-next 5/6] igb: Do not free q_vector unless new one was allocated

From: Leon Romanovsky <leon@kernel.org>
To: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: Kees Cook <keescook@chromium.org>,
	netdev@vger.kernel.org,
	"Michael J . Ruhl" <michael.j.ruhl@intel.com>,
	edumazet@google.com, intel-wired-lan@lists.osuosl.org,
	kuba@kernel.org, pabeni@redhat.com, davem@davemloft.net
Subject: Re: [Intel-wired-lan] [PATCH net-next 5/6] igb: Do not free q_vector unless new one was allocated
Date: Mon, 7 Nov 2022 09:03:06 +0200	[thread overview]
Message-ID: <Y2itqqGQm6uZ/2Wf@unreal> (raw)
In-Reply-To: <20221104205414.2354973-6-anthony.l.nguyen@intel.com>

On Fri, Nov 04, 2022 at 01:54:13PM -0700, Tony Nguyen wrote:
> From: Kees Cook <keescook@chromium.org>
> 
> Avoid potential use-after-free condition under memory pressure. If the
> kzalloc() fails, q_vector will be freed but left in the original
> adapter->q_vector[v_idx] array position.
> 
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)

You should use first and last names here.

> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index d6c1c2e66f26..c2bb658198bf 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -1202,8 +1202,12 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter,
>  	if (!q_vector) {
>  		q_vector = kzalloc(size, GFP_KERNEL);
>  	} else if (size > ksize(q_vector)) {
> -		kfree_rcu(q_vector, rcu);
> -		q_vector = kzalloc(size, GFP_KERNEL);
> +		struct igb_q_vector *new_q_vector;
> +
> +		new_q_vector = kzalloc(size, GFP_KERNEL);
> +		if (new_q_vector)
> +			kfree_rcu(q_vector, rcu);
> +		q_vector = new_q_vector;

I wonder if this is correct.
1. if new_q_vector is NULL, you will overwrite q_vector without releasing it.
2. kfree_rcu() doesn't immediately release memory, but after grace
period, but here you are overwriting the pointer which is not release
yet.

>  	} else {
>  		memset(q_vector, 0, size);
>  	}
> -- 
> 2.35.1
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan