lkml.org 
[lkml]   [2022]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH net] net: dev: Detect dev_hold() after netdev_wait_allrefs()
On Thu, Jan 27, 2022 at 5:43 PM Jann Horn <jannh@google.com> wrote:
>
> I've run into a bug where dev_hold() was being called after
> netdev_wait_allrefs(). But at that point, the device is already going
> away, and dev_hold() can't stop that anymore.
>
> To make such problems easier to diagnose in the future:
>
> - For CONFIG_PCPU_DEV_REFCNT builds: Recheck in free_netdev() whether
> the net refcount has been elevated. If this is detected, WARN() and
> leak the object (to prevent worse consequences from a
> use-after-free).
> - For builds without CONFIG_PCPU_DEV_REFCNT: Set the refcount to zero.
> This signals to the generic refcount infrastructure that any attempt
> to increment the refcount later is a bug.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> net/core/dev.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1baab07820f6..f7916c0d226d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9949,8 +9949,18 @@ void netdev_run_todo(void)
>
> netdev_wait_allrefs(dev);
>
> + /* Drop the netdev refcount (which should be 1 at this point)
> + * to zero. If we're using the generic refcount code, this will
> + * tell it that any dev_hold() after this point is a bug.
> + */
> +#ifdef CONFIG_PCPU_DEV_REFCNT
> + this_cpu_dec(*dev->pcpu_refcnt);
> + BUG_ON(netdev_refcnt_read(dev) != 0);
> +#else
> + BUG_ON(!refcount_dec_and_test(&dev->dev_refcnt));
> +#endif
> +
> /* paranoia */
> - BUG_ON(netdev_refcnt_read(dev) != 1);
> BUG_ON(!list_empty(&dev->ptype_all));
> BUG_ON(!list_empty(&dev->ptype_specific));
> WARN_ON(rcu_access_pointer(dev->ip_ptr));
> @@ -10293,6 +10303,12 @@ void free_netdev(struct net_device *dev)
> free_percpu(dev->xdp_bulkq);
> dev->xdp_bulkq = NULL;
>
> + /* Recheck in case someone called dev_hold() between
> + * netdev_wait_allrefs() and here.
> + */

At this point, dev->pcpu_refcnt per-cpu data has been freed already
(CONFIG_PCPU_DEV_REFCNT=y)

So this should probably crash, or at least UAF ?

> + if (WARN_ON(netdev_refcnt_read(dev) != 0))
> + return; /* leak memory, otherwise we might get UAF */
> +
> /* Compatibility with error handling in drivers */
> if (dev->reg_state == NETREG_UNINITIALIZED) {
> netdev_freemem(dev);
>
> base-commit: 23a46422c56144939c091c76cf389aa863ce9c18
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>

\
 
 \ /
  Last update: 2022-01-28 03:11    [W:0.110 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site