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

Singh, Krishneil K
2024-08-02T00:21:38Z

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Tuesday, July 30, 2024 9:37 AM
> To: Lobakin, Aleksander <aleksander.lobakin@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; NEX SW NCIS OSDT ITP Upstreaming
> <nex.sw.ncis.osdt.itp.upstreaming@intel.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> 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@kernel.org>
> > 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@kernel.org>
>

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