Re: [RESEND PATCH net 1/2] iavf: Fix use-after-free in free_netdev

[Ding Hui]

On 2023/4/19 3:48, Simon Horman wrote:
> Hi Ding Hui,
>
> On Mon, Apr 17, 2023 at 03:40:15PM +0800, Ding Hui wrote:
> > We do netif_napi_add() for all allocated q_vectors[], but potentially
> > do netif_napi_del() for part of them, then kfree q_vectors and lefted
>
> nit: lefted -> leave

Thanks, I'll update in v2.

> > invalid pointers at dev->napi_list.
> >
> > If num_active_queues is changed to less than allocated q_vectors[] by
> > unexpected, when iavf_remove, we might see UAF in free_netdev like this:

...

> > Fix it by letting netif_napi_del() match to netif_napi_add().
> >
> > Signed-off-by: Ding Hui <dinghui@xxxxxxxxxxxxxx>
> > Cc: Donglin Peng <pengdonglin@xxxxxxxxxxxxxx>
> > CC: Huang Cun <huangcun@xxxxxxxxxxxxxx>
>
> as this is a fix it probably should have a fixes tag.
> I wonder if it should be:
>
> Fixes: cc0529271f23 ("i40evf: don't use more queues than CPUs")

I don't think so.
I searched the git log, and found that the mismatched usage was
introduced since the beginning of i40evf_main.c, so I'll add

Fixes: 5eae00c57f5e ("i40evf: main driver core")

in v2.

> Code change looks good to me.
>
> Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>

Thanks.

And sorry for you confusion since my RESEND.

> > ---
> >   drivers/net/ethernet/intel/iavf/iavf_main.c | 6 +-----
> >   1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > index 095201e83c9d..a57e3425f960 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > @@ -1849,19 +1849,15 @@ static int iavf_alloc_q_vectors(struct iavf_adapter *adapter)
> >   static void iavf_free_q_vectors(struct iavf_adapter *adapter)
> >   {
> >   	int q_idx, num_q_vectors;
> > -	int napi_vectors;
> >   
> >   	if (!adapter->q_vectors)
> >   		return;
> >   
> >   	num_q_vectors = adapter->num_msix_vectors - NONQ_VECS;
> > -	napi_vectors = adapter->num_active_queues;
> >   
> >   	for (q_idx = 0; q_idx < num_q_vectors; q_idx++) {
> >   		struct iavf_q_vector *q_vector = &adapter->q_vectors[q_idx];
> > -
> > -		if (q_idx < napi_vectors)
> > -			netif_napi_del(&q_vector->napi);
> > +		netif_napi_del(&q_vector->napi);
> >   	}
> >   	kfree(adapter->q_vectors);
> >   	adapter->q_vectors = NULL;
> > --
> > 2.17.1

--
Thanks,
- Ding Hui