RE: [PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while performing a soft reset

[Singh, Krishneil K]

> -----Original Message-----
> From: Simon Horman <horms@xxxxxxxxxx>
> Sent: Tuesday, July 30, 2024 9:37 AM
> To: Lobakin, Aleksander <aleksander.lobakin@xxxxxxxxx>
> Cc: intel-wired-lan@xxxxxxxxxxxxxxxx; Nguyen, Anthony L
> <anthony.l.nguyen@xxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric
> Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo
> Abeni <pabeni@xxxxxxxxxx>; NEX SW NCIS OSDT ITP Upstreaming
> <nex.sw.ncis.osdt.itp.upstreaming@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH iwl-net 1/3] idpf: fix memory leaks and crashes while
> performing a soft reset
> 
> On Mon, Jul 29, 2024 at 10:54:50AM +0200, Alexander Lobakin wrote:
> > From: Simon Horman <horms@xxxxxxxxxx>
> > Date: Fri, 26 Jul 2024 17:09:54 +0100
> >
> > > On Wed, Jul 24, 2024 at 03:40:22PM +0200, Alexander Lobakin wrote:
> > >> The second tagged commit introduced a UAF, as it removed restoring
> > >> q_vector->vport pointers after reinitializating the structures.
> > >> This is due to that all queue allocation functions are performed here
> > >> with the new temporary vport structure and those functions rewrite
> > >> the backpointers to the vport. Then, this new struct is freed and
> > >> the pointers start leading to nowhere.
> >
> > [...]
> >
> > >>  err_reset:
> > >> -	idpf_vport_queues_rel(new_vport);
> > >> +	idpf_send_add_queues_msg(vport, vport->num_txq, vport-
> >num_complq,
> > >> +				 vport->num_rxq, vport->num_bufq);
> > >> +
> > >> +err_open:
> > >> +	if (current_state == __IDPF_VPORT_UP)
> > >> +		idpf_vport_open(vport);
> > >
> > > Hi Alexander,
> > >
> > > Can the system end up in an odd state if this call to idpf_vport_open(), or
> > > the one above, fails. Likewise if the above call to
> > > idpf_send_add_queues_msg() fails.
> >
> > Adding the queues with the parameters that were before changing them
> > almost can't fail. But if any of these two fails, it really will be in
> > an odd state...
> 
> Thanks for the clarification, this is my concern.
> 
> > Perhaps we need to do a more powerful reset then? Can we somehow tell
> > the kernel that in fact our iface is down, so that the user could try
> > to enable it manually once again?
> > Anyway, feels like a separate series or patch to -next, what do you think?
> 
> Yes, sure. I agree that this patch improves things, and more extreme
> corner cases can be addressed separately.
> 
> With the above in mind, I'm happy with this patch.
> 
> Reviewed-by: Simon Horman <horms@xxxxxxxxxx>
> 

Tested-by: Krishneil Singh <krishneil.k.singh@xxxxxxxxx>