Messages in this thread Patch in this message | | | From | Eric Dumazet <> | Date | Thu, 27 Jan 2022 18:24:55 -0800 | Subject | Re: [PATCH net] net: dev: Detect dev_hold() after netdev_wait_allrefs() |
| |
On Thu, Jan 27, 2022 at 6:14 PM Jann Horn <jannh@google.com> wrote: > > On Fri, Jan 28, 2022 at 3:09 AM Eric Dumazet <edumazet@google.com> wrote: > > 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 ? > > Oh. Whoops. That's what I get for only testing without CONFIG_PCPU_DEV_REFCNT... > > I guess a better place to put the new check would be directly after > checking for "dev->reg_state == NETREG_UNREGISTERING"? Like this? > > if (dev->reg_state == NETREG_UNREGISTERING) { > ASSERT_RTNL(); > dev->needs_free_netdev = true; > return; > } > > /* Recheck in case someone called dev_hold() between > * netdev_wait_allrefs() and here. > */ > if (WARN_ON(netdev_refcnt_read(dev) != 0)) > return; /* leak memory, otherwise we might get UAF */ > > netif_free_tx_queues(dev); > netif_free_rx_queues(dev);
Maybe another solution would be to leverage the recent dev_hold_track().
We could add a dead boolean to 'struct ref_tracker_dir ' (dev->refcnt_tracker)
diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index c11c9db5825cf933acf529c83db441a818135f29..d907759b2fa1dd6b2ef22f883d55963c410dc71b 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -12,6 +12,7 @@ struct ref_tracker_dir { spinlock_t lock; unsigned int quarantine_avail; refcount_t untracked; + bool dead; struct list_head list; /* List of active trackers */ struct list_head quarantine; /* List of dead trackers */ #endif @@ -24,6 +25,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, INIT_LIST_HEAD(&dir->list); INIT_LIST_HEAD(&dir->quarantine); spin_lock_init(&dir->lock); + dir->dead = false; dir->quarantine_avail = quarantine_count; refcount_set(&dir->untracked, 1); } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index a6789c0c626b0f68ad67c264cd19177a63fb82d2..cbc798e5b97674a389ea3bbf17d9bb39fbf63328 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -20,6 +20,7 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) unsigned long flags; bool leak = false;
+ dir->dead = true; spin_lock_irqsave(&dir->lock, flags); list_for_each_entry_safe(tracker, n, &dir->quarantine, head) { list_del(&tracker->head); @@ -72,6 +73,8 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, gfp_t gfp_mask = gfp; unsigned long flags;
+ WARN_ON_ONCE(dir->dead); + if (gfp & __GFP_DIRECT_RECLAIM) gfp_mask |= __GFP_NOFAIL; *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask);
| |