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

Juergen Gross <[email protected]> Thu, 8 Dec 2022 17:12:32 +0100
Newsgroups	gmane.comp.emulators.xen.announce
Message-ID	<e22fcdce-f029-de46-81a6-60f5ffc9c9a2__47289.8487512552$1670516100$gmane$org@suse.com>
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 <[email protected]>
>> 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 <[email protected]>
>> Signed-off-by: Juergen Gross <[email protected]>
>> Reviewed-by: Jan Beulich <[email protected]>
>> ---
>>   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
OpenPGP_0xB0DE9DD628BF132F.asc (application/pgp-keys, 3 KB)
OpenPGP_signature (application/pgp-signature, 495 B) - not displayed