[PATCH] xen/usb: harden xen_hcd against malicious backends

[PATCH] xen/usb: harden xen_hcd against malicious backends

[Juergen Gross]

Make sure a malicious backend can't cause any harm other than wrong
I/O data.

Missing are verification of the request id in a response, sanitizing
the reported actual I/O length, and protection against interrupt storms
from the backend.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/usb/host/xen-hcd.c | 57 ++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/xen-hcd.c b/drivers/usb/host/xen-hcd.c
index 01db5c767251..3e487baf8422 100644
--- a/drivers/usb/host/xen-hcd.c
+++ b/drivers/usb/host/xen-hcd.c
@@ -51,6 +51,7 @@ struct vdevice_status {
 struct usb_shadow {
 	struct xenusb_urb_request req;
 	struct urb *urb;
+	bool in_flight;
 };
 
 struct xenhcd_info {
@@ -720,6 +721,12 @@ static void xenhcd_gnttab_done(struct xenhcd_info *info, unsigned int id)
 	int nr_segs = 0;
 	int i;
 
+	if (!shadow->in_flight) {
+		xenhcd_set_error(info, "Illegal request id");
+		return;
+	}
+	shadow->in_flight = false;
+
 	nr_segs = shadow->req.nr_buffer_segs;
 
 	if (xenusb_pipeisoc(shadow->req.pipe))
@@ -803,6 +810,7 @@ static int xenhcd_do_request(struct xenhcd_info *info, struct urb_priv *urbp)
 
 	info->urb_ring.req_prod_pvt++;
 	info->shadow[id].urb = urb;
+	info->shadow[id].in_flight = true;
 
 	RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(&info->urb_ring, notify);
 	if (notify)
@@ -931,10 +939,27 @@ static int xenhcd_unlink_urb(struct xenhcd_info *info, struct urb_priv *urbp)
 	return ret;
 }
 
-static int xenhcd_urb_request_done(struct xenhcd_info *info)
+static void xenhcd_res_to_urb(struct xenhcd_info *info,
+			      struct xenusb_urb_response *res, struct urb *urb)
+{
+	if (unlikely(!urb))
+		return;
+
+	if (res->actual_length > urb->transfer_buffer_length)
+		urb->actual_length = urb->transfer_buffer_length;
+	else if (res->actual_length < 0)
+		urb->actual_length = 0;
+	else
+		urb->actual_length = res->actual_length;
+	urb->error_count = res->error_count;
+	urb->start_frame = res->start_frame;
+	xenhcd_giveback_urb(info, urb, res->status);
+}
+
+static int xenhcd_urb_request_done(struct xenhcd_info *info,
+				   unsigned int *eoiflag)
 {
 	struct xenusb_urb_response res;
-	struct urb *urb;
 	RING_IDX i, rp;
 	__u16 id;
 	int more_to_do = 0;
@@ -961,16 +986,12 @@ static int xenhcd_urb_request_done(struct xenhcd_info *info)
 			xenhcd_gnttab_done(info, id);
 			if (info->error)
 				goto err;
-			urb = info->shadow[id].urb;
-			if (likely(urb)) {
-				urb->actual_length = res.actual_length;
-				urb->error_count = res.error_count;
-				urb->start_frame = res.start_frame;
-				xenhcd_giveback_urb(info, urb, res.status);
-			}
+			xenhcd_res_to_urb(info, &res, info->shadow[id].urb);
 		}
 
 		xenhcd_add_id_to_freelist(info, id);
+
+		*eoiflag = 0;
 	}
 	info->urb_ring.rsp_cons = i;
 
@@ -988,7 +1009,7 @@ static int xenhcd_urb_request_done(struct xenhcd_info *info)
 	return 0;
 }
 
-static int xenhcd_conn_notify(struct xenhcd_info *info)
+static int xenhcd_conn_notify(struct xenhcd_info *info, unsigned int *eoiflag)
 {
 	struct xenusb_conn_response res;
 	struct xenusb_conn_request *req;
@@ -1033,6 +1054,8 @@ static int xenhcd_conn_notify(struct xenhcd_info *info)
 				       info->conn_ring.req_prod_pvt);
 		req->id = id;
 		info->conn_ring.req_prod_pvt++;
+
+		*eoiflag = 0;
 	}
 
 	if (rc != info->conn_ring.req_prod_pvt)
@@ -1055,14 +1078,19 @@ static int xenhcd_conn_notify(struct xenhcd_info *info)
 static irqreturn_t xenhcd_int(int irq, void *dev_id)
 {
 	struct xenhcd_info *info = (struct xenhcd_info *)dev_id;
+	unsigned int eoiflag = XEN_EOI_FLAG_SPURIOUS;
 
-	if (unlikely(info->error))
+	if (unlikely(info->error)) {
+		xen_irq_lateeoi(irq, XEN_EOI_FLAG_SPURIOUS);
 		return IRQ_HANDLED;
+	}
 
-	while (xenhcd_urb_request_done(info) | xenhcd_conn_notify(info))
+	while (xenhcd_urb_request_done(info, &eoiflag) |
+	       xenhcd_conn_notify(info, &eoiflag))
 		/* Yield point for this unbounded loop. */
 		cond_resched();
 
+	xen_irq_lateeoi(irq, eoiflag);
 	return IRQ_HANDLED;
 }
 
@@ -1139,9 +1167,9 @@ static int xenhcd_setup_rings(struct xenbus_device *dev,
 		goto fail;
 	}
 
-	err = bind_evtchn_to_irq(info->evtchn);
+	err = bind_evtchn_to_irq_lateeoi(info->evtchn);
 	if (err <= 0) {
-		xenbus_dev_fatal(dev, err, "bind_evtchn_to_irq");
+		xenbus_dev_fatal(dev, err, "bind_evtchn_to_irq_lateeoi");
 		goto fail;
 	}
 
@@ -1494,6 +1522,7 @@ static struct usb_hcd *xenhcd_create_hcd(struct xenbus_device *dev)
 	for (i = 0; i < XENUSB_URB_RING_SIZE; i++) {
 		info->shadow[i].req.id = i + 1;
 		info->shadow[i].urb = NULL;
+		info->shadow[i].in_flight = false;
 	}
 	info->shadow[XENUSB_URB_RING_SIZE - 1].req.id = 0x0fff;
 
-- 
2.34.1

Re: [PATCH] xen/usb: harden xen_hcd against malicious backends

[Greg Kroah-Hartman]

On Fri, Mar 11, 2022 at 11:35:09AM +0100, Juergen Gross wrote:
> Make sure a malicious backend can't cause any harm other than wrong
> I/O data.
> 
> Missing are verification of the request id in a response, sanitizing
> the reported actual I/O length, and protection against interrupt storms
> from the backend.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/usb/host/xen-hcd.c | 57 ++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 14 deletions(-)

Fails to apply to my tree:

checking file drivers/usb/host/xen-hcd.c
Hunk #2 succeeded at 720 (offset -1 lines).
Hunk #3 succeeded at 807 (offset -3 lines).
Hunk #4 succeeded at 934 (offset -5 lines).
Hunk #5 FAILED at 986.
Hunk #6 succeeded at 1003 with fuzz 1 (offset -10 lines).
Hunk #7 succeeded at 1048 (offset -10 lines).
Hunk #8 succeeded at 1072 (offset -10 lines).
Hunk #9 succeeded at 1161 (offset -10 lines).
Hunk #10 succeeded at 1516 (offset -10 lines).
1 out of 10 hunks FAILED

Any hints?

thanks,

greg k-h

Re: [PATCH] xen/usb: harden xen_hcd against malicious backends

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1255 bytes --]

On 15.03.22 18:41, Greg Kroah-Hartman wrote:
> On Fri, Mar 11, 2022 at 11:35:09AM +0100, Juergen Gross wrote:
>> Make sure a malicious backend can't cause any harm other than wrong
>> I/O data.
>>
>> Missing are verification of the request id in a response, sanitizing
>> the reported actual I/O length, and protection against interrupt storms
>> from the backend.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   drivers/usb/host/xen-hcd.c | 57 ++++++++++++++++++++++++++++----------
>>   1 file changed, 43 insertions(+), 14 deletions(-)
> 
> Fails to apply to my tree:
> 
> checking file drivers/usb/host/xen-hcd.c
> Hunk #2 succeeded at 720 (offset -1 lines).
> Hunk #3 succeeded at 807 (offset -3 lines).
> Hunk #4 succeeded at 934 (offset -5 lines).
> Hunk #5 FAILED at 986.
> Hunk #6 succeeded at 1003 with fuzz 1 (offset -10 lines).
> Hunk #7 succeeded at 1048 (offset -10 lines).
> Hunk #8 succeeded at 1072 (offset -10 lines).
> Hunk #9 succeeded at 1161 (offset -10 lines).
> Hunk #10 succeeded at 1516 (offset -10 lines).
> 1 out of 10 hunks FAILED
> 
> Any hints?

Rebase your tree to v5.17-rc8? It is missing the recent security
patches which modified drivers/usb/host/xen-hcd.c.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] xen/usb: harden xen_hcd against malicious backends

[Greg Kroah-Hartman]

On Wed, Mar 16, 2022 at 06:29:00AM +0100, Juergen Gross wrote:
> On 15.03.22 18:41, Greg Kroah-Hartman wrote:
> > On Fri, Mar 11, 2022 at 11:35:09AM +0100, Juergen Gross wrote:
> > > Make sure a malicious backend can't cause any harm other than wrong
> > > I/O data.
> > > 
> > > Missing are verification of the request id in a response, sanitizing
> > > the reported actual I/O length, and protection against interrupt storms
> > > from the backend.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > ---
> > >   drivers/usb/host/xen-hcd.c | 57 ++++++++++++++++++++++++++++----------
> > >   1 file changed, 43 insertions(+), 14 deletions(-)
> > 
> > Fails to apply to my tree:
> > 
> > checking file drivers/usb/host/xen-hcd.c
> > Hunk #2 succeeded at 720 (offset -1 lines).
> > Hunk #3 succeeded at 807 (offset -3 lines).
> > Hunk #4 succeeded at 934 (offset -5 lines).
> > Hunk #5 FAILED at 986.
> > Hunk #6 succeeded at 1003 with fuzz 1 (offset -10 lines).
> > Hunk #7 succeeded at 1048 (offset -10 lines).
> > Hunk #8 succeeded at 1072 (offset -10 lines).
> > Hunk #9 succeeded at 1161 (offset -10 lines).
> > Hunk #10 succeeded at 1516 (offset -10 lines).
> > 1 out of 10 hunks FAILED
> > 
> > Any hints?
> 
> Rebase your tree to v5.17-rc8? It is missing the recent security
> patches which modified drivers/usb/host/xen-hcd.c.

I can't rebase, but I can merge.  I'll do that, thanks.

greg k-h