vhost_vsock_alloc_skb() allocates an skb from the total guest descriptor
length before reading hdr->len. Descriptor capacity and declared payload
length are independent, so a guest can supply a large descriptor with a
zero or short payload.
On Linux v7.2, 455 zero-payload packets with 64 KiB descriptors retained
30,255,680 bytes on a 256 KiB receive buffer while rx_bytes and buf_used
stayed zero. A full-payload control retained 265,984 bytes. The existing
SKB_TRUESIZE(0) budget caps skb count but does not account for the
descriptor-sized allocation.
Repeating this across connections can exhaust host kernel memory.
Trimming after allocation is insufficient. Linear skbs retain their full
head, while a nonlinear payload exceeding head tailroom can retain its
first page fragment.
Copy the header into a stack object, validate hdr->len before allocating,
and size the skb from the declared payload plus the header. This keeps the
allocation proportional to receive accounting for both linear and
nonlinear skbs.
Use payload_len > len - sizeof(hdr) for validation. This avoids addition
overflow on 32-bit hosts and ensures payload_len fits the subsequent
int-length copy path.
Fixes: ab9aa2f3afc2 ("vhost/vsock: Allocate nonlinear SKBs for handling large
receive buffers")
Cc: [email protected]
Assisted-by: LLM
Signed-off-by: Daehyeon Ko <[email protected]>
---
Changes in v2:
- validate the header before allocation with an overflow-safe bounds check;
- size the skb from declared payload and remove the global trim-helper
change;
- remove the generic clean-validation statement from the changelog; and
- add Will Deacon to Cc.
v1: https://lore.kernel.org/netdev/[email protected]/
Built and booted on v7.2 with KASAN, UBSAN and LOCKDEP. A focused
allocation/queue probe retained 436,800 and 582,400 bytes for 455 zero and
341-byte packets, versus 30,255,680 bytes for the vulnerable zero case. No
KASAN report, WARN, oops or panic occurred. The probe injects skbs
in-kernel rather than through a live guest virtqueue.
drivers/vhost/vsock.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index abed1fbcf66cc5..35f75e23c7497e 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -364,7 +364,7 @@ static struct sk_buff *
vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
unsigned int out, unsigned int in)
{
- struct virtio_vsock_hdr *hdr;
+ struct virtio_vsock_hdr hdr;
struct iov_iter iov_iter;
struct sk_buff *skb;
size_t payload_len;
@@ -382,34 +382,32 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + VIRTIO_VSOCK_SKB_HEADROOM)
return NULL;
- /* len contains both payload and hdr */
- skb = virtio_vsock_alloc_skb(len, GFP_KERNEL);
- if (!skb)
- return NULL;
-
iov_iter_init(&iov_iter, ITER_SOURCE, vq->iov, out, len);
- hdr = virtio_vsock_hdr(skb);
- nbytes = copy_from_iter(hdr, sizeof(*hdr), &iov_iter);
- if (nbytes != sizeof(*hdr)) {
+ nbytes = copy_from_iter(&hdr, sizeof(hdr), &iov_iter);
+ if (nbytes != sizeof(hdr)) {
vq_err(vq, "Expected %zu bytes for pkt->hdr, got %zu bytes\n",
- sizeof(*hdr), nbytes);
- kfree_skb(skb);
+ sizeof(hdr), nbytes);
return NULL;
}
- payload_len = le32_to_cpu(hdr->len);
+ payload_len = le32_to_cpu(hdr.len);
+
+ /* The pkt is too big or the length in the header is invalid */
+ if (payload_len > len - sizeof(hdr))
+ return NULL;
+
+ /* Allocate only for the payload declared in the header. */
+ skb = virtio_vsock_alloc_skb(payload_len + sizeof(hdr), GFP_KERNEL);
+ if (!skb)
+ return NULL;
+
+ memcpy(virtio_vsock_hdr(skb), &hdr, sizeof(hdr));
/* No payload */
if (!payload_len)
return skb;
- /* The pkt is too big or the length in the header is invalid */
- if (payload_len + sizeof(*hdr) > len) {
- kfree_skb(skb);
- return NULL;
- }
-
virtio_vsock_skb_put(skb, payload_len);
if (skb_copy_datagram_from_iter(skb, 0, &iov_iter, payload_len)) {
base-commit: 6dc989ea46b96ce170840174b4a38c4a387fb005
--
2.55.0
