Re: [PATCH net] nfp: flower: fix use-after-free in neighbour update work

[Daniel Machon]

> The netevent notifier handles neighbour updates and redirects by
> queuing nfp_tun_neigh_update() on system_highpri_wq. Each work item
> holds a reference to the neighbour, but only stores a pointer to the
> flower app.
> 
> During device removal, nfp_tunnel_config_stop() unregisters the
> notifier. This waits for notifier callbacks to return, but does not
> wait for the work they queued. The teardown then frees app->priv and
> app without waiting for these work items. Flushing cmsg_work in
> nfp_flower_clean() does not cover the neighbour update work.
> 
> A pending work item can therefore run after the app has been freed.
> Its call to nfp_flower_get_port_id_from_netdev() can then access freed
> memory while checking whether the neighbour's device can be offloaded.
> Even notifications for unrelated devices can reach this check. The
> same problem affects shutdown and probe error paths that tear down the
> app after the netevent notifier has been registered.
> 
> One possible interleaving is shown below. The neighbour belongs to
> another network device that remains registered.
> 
>   CPU0 (remove)                   CPU1 (notifier / kworker)
>                                   nfp_tun_neigh_event_handler()
>                                     queue_work(system_highpri_wq, ...)
>                                     return
>                                   /* Work has not run yet. */
>   nfp_tunnel_config_stop()
>     unregister_netevent_notifier()
>     /* Notifier callbacks have finished, but work may be pending. */
>   nfp_flower_clean()
>     vfree(app->priv)
>     app->priv = NULL
>   nfp_app_free()
>     kfree(app)
>                                   nfp_tun_neigh_update()
>                                     app = update_work->app
>                                     nfp_flower_get_port_id_from_netdev()
>                                       app->priv  /* use-after-free */
> 
> The work items are allocated per event and free themselves, so they
> cannot be cancelled individually, and flushing system_highpri_wq is
> not allowed. Use a dedicated workqueue for neighbour updates instead.
> In nfp_tunnel_config_stop(), unregister the notifier and destroy the
> workqueue before cleaning up the tunnel state. This prevents new work
> from being queued and waits for pending and running work to finish
> while the app and its resources are still valid.
> 
> Found by code inspection.
> 
> Fixes: 0ad722bd9ee3 ("nfp: flower: fix for take a mutex lock in soft irq context and rcu lock")
> Signed-off-by: Sicong Huang <congei42@xxxxxxx>

Looks right to me.

Reviewed-by: Daniel Machon <daniel.machon@xxxxxxxxxxxxx>