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

Simon Horman
2024-07-30T11:03:29Z

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...
>
> 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?
>
> >
> >> +
> >> free_vport:
> >> kfree(new_vport);
>
> Thanks,
> Olek
>