[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Xen Security Advisory 424 v1 (CVE-2022-42328,CVE-2022-42329) - Guests can trigger deadlock in Linux netback driver



On 08.12.22 16:59, Pratyush Yadav wrote:

Hi,

I noticed one interesting thing about this patch but I'm not familiar
enough with the driver to say for sure what the right thing is.

On Tue, Dec 06 2022, Xen.org security team wrote:

[...]

 From cfdf8fd81845734b6152b4617746c1127ec52228 Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@xxxxxxxx>
Date: Tue, 6 Dec 2022 08:54:24 +0100
Subject: [PATCH] xen/netback: don't call kfree_skb() with interrupts disabled

It is not allowed to call kfree_skb() from hardware interrupt
context or with interrupts being disabled. So remove kfree_skb()
from the spin_lock_irqsave() section and use the already existing
"drop" label in xenvif_start_xmit() for dropping the SKB. At the
same time replace the dev_kfree_skb() call there with a call of
dev_kfree_skb_any(), as xenvif_start_xmit() can be called with
disabled interrupts.

This is XSA-424 / CVE-2022-42328 / CVE-2022-42329.

Fixes: be81992f9086 ("xen/netback: don't queue unlimited number of packages")
Reported-by: Yang Yingliang <yangyingliang@xxxxxxxxxx>
Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
---
  drivers/net/xen-netback/common.h    | 2 +-
  drivers/net/xen-netback/interface.c | 6 ++++--
  drivers/net/xen-netback/rx.c        | 8 +++++---
  3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 1545cbee77a4..3dbfc8a6924e 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -386,7 +386,7 @@ int xenvif_dealloc_kthread(void *data);
  irqreturn_t xenvif_ctrl_irq_fn(int irq, void *data);

  bool xenvif_have_rx_work(struct xenvif_queue *queue, bool test_kthread);
-void xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb);
+bool xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb);

  void xenvif_carrier_on(struct xenvif *vif);

diff --git a/drivers/net/xen-netback/interface.c 
b/drivers/net/xen-netback/interface.c
index 650fa180220f..f3f2c07423a6 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -254,14 +254,16 @@ xenvif_start_xmit(struct sk_buff *skb, struct net_device 
*dev)
        if (vif->hash.alg == XEN_NETIF_CTRL_HASH_ALGORITHM_NONE)
                skb_clear_hash(skb);

-       xenvif_rx_queue_tail(queue, skb);
+       if (!xenvif_rx_queue_tail(queue, skb))
+               goto drop;
+
        xenvif_kick_thread(queue);

        return NETDEV_TX_OK;

   drop:
        vif->dev->stats.tx_dropped++;

Now tx_dropped is incremented on packet drop...

-       dev_kfree_skb(skb);
+       dev_kfree_skb_any(skb);
        return NETDEV_TX_OK;
  }

diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
index 932762177110..0ba754ebc5ba 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -82,9 +82,10 @@ static bool xenvif_rx_ring_slots_available(struct 
xenvif_queue *queue)
        return false;
  }

-void xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb)
+bool xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb)
  {
        unsigned long flags;
+       bool ret = true;

        spin_lock_irqsave(&queue->rx_queue.lock, flags);

@@ -92,8 +93,7 @@ void xenvif_rx_queue_tail(struct xenvif_queue *queue, struct 
sk_buff *skb)
                struct net_device *dev = queue->vif->dev;

                netif_tx_stop_queue(netdev_get_tx_queue(dev, queue->id));
-               kfree_skb(skb);
-               queue->vif->dev->stats.rx_dropped++;

... but earlier rx_dropped was incremented.

Which one is actually correct? This line was added by be81992f9086b
("xen/netback: don't queue unlimited number of packages"), which was the
fix for XSA-392. I think incrementing tx_dropped is the right thing to
do, as was done before XSA-392 but it would be nice if someone else
takes a look at this as well.

Yes, I think the XSA-392 patch was wrong in this regard.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.