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 ]
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.

> + ret = false;
> } else {
> if (skb_queue_empty(&queue->rx_queue))
> xenvif_update_needed_slots(queue, skb);
> @@ -104,6 +104,8 @@ void xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb)
> }
>
> spin_unlock_irqrestore(&queue->rx_queue.lock, flags);
> +
> + return ret;
> }
>
> static struct sk_buff *xenvif_rx_dequeue(struct xenvif_queue *queue)

--
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
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
Re: Xen Security Advisory 424 v1 (CVE-2022-42328,CVE-2022-42329) - Guests can trigger deadlock in Linux netback driver [ In reply to ]
On Thu, Dec 8, 2022 at 4:13 PM Juergen Gross <jgross@suse.com> wrote:
>
> 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.
>

Netback calls this rx (to-guest) traffic so rx_dropped seems better. On the
other hand, the networking stack thinks of this as tx since the packet is going
from the networking stack to the NIC driver...

Regardless, it is currently inconsistent since to-guest traffic increments
tx_dropped if it is dropped because the rx queue len is too long but it
increments rx_dropped if those same packets are dropped when they expire in the
rx queue.

I also see that the tx path (from-guest) doesn't increment any dropped counters
when it drops a packet.

Ross