[PATCH] virtio_net: fix use after free

Discussion:

Michael S. Tsirkin

2014-10-15 13:23:28 UTC

commit 0b725a2ca61bedc33a2a63d0451d528b268cf975
net: Remove ndo_xmit_flush netdev operation, use signalling instead.

added code that looks at skb->xmit_more after the skb has
been put in TX VQ. Since some paths process the ring and free the skb
immediately, this can cause use after free.

Fix by storing xmit_more in a local variable.

Cc: David S. Miller <***@davemloft.net>
Signed-off-by: Michael S. Tsirkin <***@redhat.com>
---

David, am I using the API correctly?
Seems to work for me.
You used __netif_subqueue_stopped but that seems to use
a slightly more expensive test_bit internally.
The reason I added a variable for the txq here is because it's handy for
BQL patch later on.

drivers/net/virtio_net.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3d0ce44..13d0a8b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -920,6 +920,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
int qnum = skb_get_queue_mapping(skb);
struct send_queue *sq = &vi->sq[qnum];
int err;
+ struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
+ bool kick = !skb->xmit_more;

/* Free up any pending old buffers before queueing new ones. */
free_old_xmit_skbs(sq);
@@ -956,7 +958,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
}
}

- if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more)
+ if (kick || netif_xmit_stopped(txq))
virtqueue_kick(sq->vq);

return NETDEV_TX_OK;

--
MST

David Laight

2014-10-15 13:24:57 UTC

Permalink

From: Michael S. Tsirkin

Post by Michael S. Tsirkin
commit 0b725a2ca61bedc33a2a63d0451d528b268cf975
net: Remove ndo_xmit_flush netdev operation, use signalling instead.
added code that looks at skb->xmit_more after the skb has
been put in TX VQ. Since some paths process the ring and free the skb
immediately, this can cause use after free.
Fix by storing xmit_more in a local variable.
---
David, am I using the API correctly?
Seems to work for me.
You used __netif_subqueue_stopped but that seems to use
a slightly more expensive test_bit internally.
The reason I added a variable for the txq here is because it's handy for
BQL patch later on.
drivers/net/virtio_net.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3d0ce44..13d0a8b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -920,6 +920,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
int qnum = skb_get_queue_mapping(skb);
struct send_queue *sq = &vi->sq[qnum];
int err;
+ struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);

Do you need to cache 'txq' on stack for the entire call?
Looks like it is only needed when 'kick' is true.
I've not looked to see if saves both 'dev' and 'qnum' being kept.

In any case it isn't mentioned in the commit message.

David

Post by Michael S. Tsirkin
+ bool kick = !skb->xmit_more;
/* Free up any pending old buffers before queueing new ones. */
free_old_xmit_skbs(sq);
@@ -956,7 +958,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
}
}
- if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more)
+ if (kick || netif_xmit_stopped(txq))
virtqueue_kick(sq->vq);
return NETDEV_TX_OK;
--
MST
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michael S. Tsirkin

2014-10-15 13:37:10 UTC

Permalink

Post by David Laight
From: Michael S. Tsirkin

Do you need to cache 'txq' on stack for the entire call?
Looks like it is only needed when 'kick' is true.
I've not looked to see if saves both 'dev' and 'qnum' being kept.
In any case it isn't mentioned in the commit message.
David

Code seems slightly neater this way, I haven't bothered to
micro-optimize it to this level yet.
Want to benchmark and send a patch on top?

Post by David Laight

David Miller

2014-10-15 20:47:27 UTC

Permalink

From: "Michael S. Tsirkin" <***@redhat.com>
Date: Wed, 15 Oct 2014 16:23:28 +0300

Post by Michael S. Tsirkin
You used __netif_subqueue_stopped but that seems to use
a slightly more expensive test_bit internally.

More expensive in what sense? It should be roughly the same
as "x & y" sans the volatile.

Anyways I'm ambivalent and I want to see this bug fixes, so I'll
apply your patch.

Thanks!

Michael S. Tsirkin

2014-10-17 09:20:11 UTC

Permalink

Post by David Miller
Date: Wed, 15 Oct 2014 16:23:28 +0300

Post by Michael S. Tsirkin
You used __netif_subqueue_stopped but that seems to use
a slightly more expensive test_bit internally.

More expensive in what sense? It should be roughly the same
as "x & y" sans the volatile.

I really just meant volatile - this might prevent some compiler
optimizations. I have't actually checked the produced binary so
I don't know for sure.

Post by David Miller
Anyways I'm ambivalent and I want to see this bug fixes, so I'll
apply your patch.
Thanks!