[PATCH] usbip: vhci: validate number_of_packets in RET_SUBMIT response

[Sebastian Josue Alba Vives <sebasjosue84@gmail.com>]

From: Sebastián Alba Vives <sebasjosue84@gmail.com>

vhci_recv_ret_submit() calls usbip_pack_pdu() which overwrites
urb->number_of_packets with the value from the network PDU reply
without any validation. A malicious USB/IP server can set
number_of_packets to a value larger than the original URB allocation,
causing usbip_recv_iso() and usbip_pad_iso() to access
urb->iso_frame_desc[] entries beyond the allocated array.

This leads to a heap buffer overflow in kernel memory, reachable over
the network without authentication.

The attack chain is:
  1. Client sends isochronous URB with number_of_packets = N
  2. Server replies with number_of_packets = N' >> N
  3. usbip_pack_pdu() blindly copies N' into urb->number_of_packets
  4. usbip_recv_iso() loops N' times over iso_frame_desc[N] → OOB
  5. usbip_pad_iso() also loops N' times → second OOB

Save the original number_of_packets before usbip_pack_pdu() and
validate the returned value does not exceed it. Also add a defensive
bounds check in usbip_recv_iso() against USBIP_MAX_ISO_PACKETS and
use array_size() to prevent integer overflow in the allocation.

Note that stub_rx.c already validates number_of_packets against
USBIP_MAX_ISO_PACKETS for CMD_SUBMIT on the server side, but no
equivalent validation existed on the client side for RET_SUBMIT.

Cc: stable@vger.kernel.org
Signed-off-by: Sebastián Alba Vives <sebasjosue84@gmail.com>
---
 drivers/usb/usbip/usbip_common.c | 18 ++++++++++++++----
 drivers/usb/usbip/vhci_rx.c      | 27 +++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index a2b2da125..f1eeab3a5 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -662,7 +662,6 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)
 	void *buff;
 	struct usbip_iso_packet_descriptor *iso;
 	int np = urb->number_of_packets;
-	int size = np * sizeof(*iso);
 	int i;
 	int ret;
 	int total_length = 0;
@@ -674,12 +673,23 @@ int usbip_recv_iso(struct usbip_device *ud, struct urb *urb)
 	if (np == 0)
 		return 0;
 
-	buff = kzalloc(size, GFP_KERNEL);
+	/*
+	 * Sanity check to prevent heap overflow. USBIP_MAX_ISO_PACKETS
+	 * is validated on the stub (server) side for CMD_SUBMIT, but the
+	 * client must also validate in case of a malicious server reply.
+	 * Also rejects negative values.
+	 */
+	if (np < 0 || np > USBIP_MAX_ISO_PACKETS) {
+		dev_err(&urb->dev->dev, "recv iso: invalid np %d\n", np);
+		return -EPROTO;
+	}
+
+	buff = kzalloc(array_size(np, sizeof(*iso)), GFP_KERNEL);
 	if (!buff)
 		return -ENOMEM;
 
-	ret = usbip_recv(ud->tcp_socket, buff, size);
-	if (ret != size) {
+	ret = usbip_recv(ud->tcp_socket, buff, np * sizeof(*iso));
+	if (ret != np * sizeof(*iso)) {
 		dev_err(&urb->dev->dev, "recv iso_frame_descriptor, %d\n",
 			ret);
 		kfree(buff);
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index a75f4a898..4ca7cda62 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -60,6 +60,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
 	struct usbip_device *ud = &vdev->ud;
 	struct urb *urb;
 	unsigned long flags;
+	int orig_number_of_packets;
 
 	spin_lock_irqsave(&vdev->priv_lock, flags);
 	urb = pickup_urb_and_free_priv(vdev, pdu->base.seqnum);
@@ -73,9 +74,35 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
 		return;
 	}
 
+	/*
+	 * Save the original number_of_packets before usbip_pack_pdu()
+	 * overwrites it with the value from the network PDU. The
+	 * iso_frame_desc array was allocated based on this original value,
+	 * so the reply must not claim more packets than were originally
+	 * submitted.
+	 */
+	orig_number_of_packets = urb->number_of_packets;
+
 	/* unpack the pdu to a urb */
 	usbip_pack_pdu(pdu, urb, USBIP_RET_SUBMIT, 0);
 
+	/*
+	 * Validate that the server did not return more isochronous packets
+	 * than were originally submitted. A malicious server could set
+	 * number_of_packets to a value larger than the original, causing
+	 * usbip_recv_iso() and usbip_pad_iso() to access iso_frame_desc
+	 * entries beyond the allocated array, resulting in heap buffer
+	 * overflow.
+	 */
+	if (urb->number_of_packets < 0 ||
+	    urb->number_of_packets > orig_number_of_packets) {
+		dev_err(&urb->dev->dev,
+			"pdu number_of_packets %d > original %d\n",
+			urb->number_of_packets, orig_number_of_packets);
+		urb->status = -EPROTO;
+		goto error;
+	}
+
 	/* recv transfer buffer */
 	if (usbip_recv_xbuff(ud, urb) < 0) {
 		urb->status = -EPROTO;
-- 
2.43.0