Re: [PATCH v2 1/2] neigh: fix possible DoS due to net iface start/stop loop

[Denis V. Lunev]

On 15.08.2022 11:44, Christian Brauner wrote:
> On Wed, Aug 10, 2022 at 07:08:39PM +0300, Alexander Mikhalitsyn wrote:
> > From: "Denis V. Lunev" <den@xxxxxxxxxx>
> >
> > Normal processing of ARP request (usually this is Ethernet broadcast
> > packet) coming to the host is looking like the following:
> > * the packet comes to arp_process() call and is passed through routing
> >    procedure
> > * the request is put into the queue using pneigh_enqueue() if
> >    corresponding ARP record is not local (common case for container
> >    records on the host)
> > * the request is processed by timer (within 80 jiffies by default) and
> >    ARP reply is sent from the same arp_process() using
> >    NEIGH_CB(skb)->flags & LOCALLY_ENQUEUED condition (flag is set inside
> >    pneigh_enqueue())
> >
> > And here the problem comes. Linux kernel calls pneigh_queue_purge()
> > which destroys the whole queue of ARP requests on ANY network interface
> > start/stop event through __neigh_ifdown().
> >
> > This is actually not a problem within the original world as network
> > interface start/stop was accessible to the host 'root' only, which
> > could do more destructive things. But the world is changed and there
> > are Linux containers available. Here container 'root' has an access
> > to this API and could be considered as untrusted user in the hosting
> > (container's) world.
> >
> > Thus there is an attack vector to other containers on node when
> > container's root will endlessly start/stop interfaces. We have observed
> > similar situation on a real production node when docker container was
> > doing such activity and thus other containers on the node become not
> > accessible.
> >
> > The patch proposed doing very simple thing. It drops only packets from
> > the same namespace in the pneigh_queue_purge() where network interface
> > state change is detected. This is enough to prevent the problem for the
> > whole node preserving original semantics of the code.
> This is how I'd do it as well.
>
> > v2:
> > 	- do del_timer_sync() if queue is empty after pneigh_queue_purge()
> >
> > Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> > Cc: Eric Dumazet <edumazet@xxxxxxxxxx>
> > Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> > Cc: Paolo Abeni <pabeni@xxxxxxxxxx>
> > Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> > Cc: David Ahern <dsahern@xxxxxxxxxx>
> > Cc: Yajun Deng <yajun.deng@xxxxxxxxx>
> > Cc: Roopa Prabhu <roopa@xxxxxxxxxx>
> > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > Cc: netdev@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: Alexey Kuznetsov <kuznet@xxxxxxxxxxxxx>
> > Cc: Alexander Mikhalitsyn <alexander.mikhalitsyn@xxxxxxxxxxxxx>
> > Cc: Konstantin Khorenko <khorenko@xxxxxxxxxxxxx>
> > Cc: kernel@xxxxxxxxxx
> > Cc: devel@xxxxxxxxxx
> > Investigated-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@xxxxxxxxxxxxx>
> > Signed-off-by: Denis V. Lunev <den@xxxxxxxxxx>
> > ---
> >   net/core/neighbour.c | 25 +++++++++++++++++--------
> >   1 file changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 54625287ee5b..19d99d1eff53 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -307,14 +307,23 @@ static int neigh_del_timer(struct neighbour *n)
> >   	return 0;
> >   }
> >   
> > -static void pneigh_queue_purge(struct sk_buff_head *list)
> > +static void pneigh_queue_purge(struct sk_buff_head *list, struct net *net)
> >   {
> > +	unsigned long flags;
> >   	struct sk_buff *skb;
> >   
> > -	while ((skb = skb_dequeue(list)) != NULL) {
> > -		dev_put(skb->dev);
> > -		kfree_skb(skb);
> > +	spin_lock_irqsave(&list->lock, flags);
> I'm a bit surprised to see a spinlock held around a while loop walking a
> linked list but that seems to be quite common in this file. I take it
> the lists are guaranteed to be short.
Within the current code the size of the list is 64 packets at most
(same spinlock is held during packets processing).

Though this semantics is changed in the next patch from
Alexander, where we will get 64 packets/interface.

Den

> > +	skb = skb_peek(list);
> > +	while (skb != NULL) {
> > +		struct sk_buff *skb_next = skb_peek_next(skb, list);
> > +		if (net == NULL || net_eq(dev_net(skb->dev), net)) {
> > +			__skb_unlink(skb, list);
> > +			dev_put(skb->dev);
> > +			kfree_skb(skb);
> > +		}
> > +		skb = skb_next;
> >   	}
> > +	spin_unlock_irqrestore(&list->lock, flags);
> >   }
> >   
> >   static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
> > @@ -385,9 +394,9 @@ static int __neigh_ifdown(struct neigh_table *tbl, struct net_device *dev,
> >   	write_lock_bh(&tbl->lock);
> >   	neigh_flush_dev(tbl, dev, skip_perm);
> >   	pneigh_ifdown_and_unlock(tbl, dev);
> > -
> > -	del_timer_sync(&tbl->proxy_timer);
> > -	pneigh_queue_purge(&tbl->proxy_queue);
> > +	pneigh_queue_purge(&tbl->proxy_queue, dev_net(dev));
> > +	if (skb_queue_empty_lockless(&tbl->proxy_queue))
> > +		del_timer_sync(&tbl->proxy_timer);
> >   	return 0;
> >   }
> >   
> > @@ -1787,7 +1796,7 @@ int neigh_table_clear(int index, struct neigh_table *tbl)
> >   	cancel_delayed_work_sync(&tbl->managed_work);
> >   	cancel_delayed_work_sync(&tbl->gc_work);
> >   	del_timer_sync(&tbl->proxy_timer);
> > -	pneigh_queue_purge(&tbl->proxy_queue);
> > +	pneigh_queue_purge(&tbl->proxy_queue, NULL);
> >   	neigh_ifdown(tbl, NULL);
> >   	if (atomic_read(&tbl->entries))
> >   		pr_crit("neighbour leakage\n");
> > --
> > 2.36.1