Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions vwifi.c
Original file line number Diff line number Diff line change
Expand Up @@ -3143,6 +3143,7 @@ static netdev_tx_t vwifi_virtio_tx(struct vwifi_vif *vif, struct sk_buff *skb)
struct virtio_net_hdr_mrg_rxbuf *hdr =
(struct virtio_net_hdr_mrg_rxbuf *) skb->cb;
struct scatterlist sg[2];
unsigned int pkt_len = skb->len;
int err;
unsigned long flags;

Expand All @@ -3168,11 +3169,14 @@ static netdev_tx_t vwifi_virtio_tx(struct vwifi_vif *vif, struct sk_buff *skb)
pr_info("%s: virtqueue_kick fail\n", __func__);
goto out_free;
}
vif->stats.tx_packets++;
vif->stats.tx_bytes += pkt_len;

spin_unlock_irqrestore(&vwifi_virtio_lock, flags);
return NETDEV_TX_OK;

out_free:
vif->stats.tx_dropped++;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: This path incorrectly counts dropped packets when virtqueue_kick() suppresses a notification. In the Linux virtio API, virtqueue_kick() returning false means the notification was not needed (suppressed), not that transmission failed. After virtqueue_add_outbuf() succeeds, the skb is owned by the virtqueue and the completion handler (vwifi_virtio_tx_done) will free it. Jumping to out_free here will both miscount tx_dropped and free an skb still owned by the virtqueue, risking a double-free or UAF when TX completion later frees it.

The established kernel pattern — as fixed in virtio_net.c — is to call virtqueue_kick() without checking its return value, because the buffers are already committed to the virtqueue regardless of whether a notification is sent.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vwifi.c, line 3179:

<comment>This path incorrectly counts dropped packets when `virtqueue_kick()` suppresses a notification. In the Linux virtio API, `virtqueue_kick()` returning `false` means the notification was not needed (suppressed), not that transmission failed. After `virtqueue_add_outbuf()` succeeds, the `skb` is owned by the virtqueue and the completion handler (`vwifi_virtio_tx_done`) will free it. Jumping to `out_free` here will both miscount `tx_dropped` and free an skb still owned by the virtqueue, risking a double-free or UAF when TX completion later frees it.

The established kernel pattern — as fixed in virtio_net.c — is to call `virtqueue_kick()` without checking its return value, because the buffers are already committed to the virtqueue regardless of whether a notification is sent.</comment>

<file context>
@@ -3168,11 +3169,14 @@ static netdev_tx_t vwifi_virtio_tx(struct vwifi_vif *vif, struct sk_buff *skb)
     return NETDEV_TX_OK;
 
 out_free:
+    vif->stats.tx_dropped++; 
     spin_unlock_irqrestore(&vwifi_virtio_lock, flags);
     dev_kfree_skb(skb);
</file context>

spin_unlock_irqrestore(&vwifi_virtio_lock, flags);
dev_kfree_skb(skb);
return err;
Expand Down