Mailing List Archive

Xen Security Advisory 424 v1 (CVE-2022-42328,CVE-2022-42329) - Guests can trigger deadlock in Linux netback driver
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Xen Security Advisory CVE-2022-42328,CVE-2022-42329 / XSA-424

Guests can trigger deadlock in Linux netback driver

ISSUE DESCRIPTION
=================

The patch for XSA-392 introduced another issue which might result in
a deadlock when trying to free the SKB of a packet dropped due to
the XSA-392 handling (CVE-2022-42328).

Additionally when dropping packages for other reasons the same
deadlock could occur in case of netpoll being active for the interface
the xen-netback driver is connected to (CVE-2022-42329).

IMPACT
======

A malicious guest could cause Denial of Service (DoS) of the host via
the paravirtualized network interface.

VULNERABLE SYSTEMS
==================

All systems using the Linux kernel based network backend xen-netback
are vulnerable.

MITIGATION
==========

Using another PV network backend (e.g. the qemu based "qnic" backend)
will mitigate the problem.

Using a dedicated network driver domain per guest will mitigate the
problem.

NOTE REGARDING LACK OF EMBARGO
==============================

This issue was discussed in public already.

RESOLUTION
==========

Applying the attached patch resolves this issue.

xsa424-linux.patch Linux 6.0, 6.1-rc

$ sha256sum xsa424*
89db7cad9694f498c4ac450356932fb69fb514162e07aea0343776effa821fc8 xsa424-linux.patch
$

-----BEGIN PGP SIGNATURE-----

iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAmOPXKYMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZ30IH/1GZwPXXAqMjN3d1n7BotiDLfmDiNp8e92wvQvmh
cXgsBtvTZ+oDzI7J+Xr/42c4IN41s34fWl0hmNbdrw4lwrOSoj0rnCP73Bn22oUT
jbv3bmFOHytCs5crvVrA4S7dCNcdpoEmfOoSaz1cBPhMecotlgTQo7M2Cagv3O9a
a9fR+KGMk9EBDGdo2wBJyEcD9ApASPEV+LJgLoTOuYFIStCO/+TTBfJx5H7T/vgK
Dqxsq1nULCSBc5Z5wrmtF49G3asBrAbPTkRhpyp9giXU+UV0QNJclnc+IJPdLIOe
jISAvpHQ3Fkb7Q25jaBg+c0bf9KzT3ekBOaf1RofgA84Jg0=
=4J/5
-----END PGP SIGNATURE-----
Re: Xen Security Advisory 424 v1 (CVE-2022-42328,CVE-2022-42329) - Guests can trigger deadlock in Linux netback driver [ In reply to ]
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@suse.com>
>> 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@huawei.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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