* [Qemu-devel] [PATCH] usb: xHCI: add check to limit command TRB processing
@ 2016-10-06 5:50 P J P
2016-10-07 8:22 ` Gerd Hoffmann
0 siblings, 1 reply; 3+ messages in thread
From: P J P @ 2016-10-06 5:50 UTC (permalink / raw)
To: Qemu Developers; +Cc: Gerd Hoffmann, Li Qiang, Prasad J Pandit
From: Prasad J Pandit <pjp@fedoraproject.org>
USB xHCI controller uses ring of Transfer Request Blocks(TRB)
to process USB commands. These are processed by loop in
'xhci_ring_fetch'. A guest user could make it read and process
a same TRB infinitely. Limit number of command TRBs to avoid it.
Reported-by: Li Qiang <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
hw/usb/hcd-xhci.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 188f954..17dba2a 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -140,6 +140,7 @@
#define ERDP_EHB (1<<3)
#define TRB_SIZE 16
+#define TRB_MAX 4000
typedef struct XHCITRB {
uint64_t parameter;
uint32_t status;
@@ -999,9 +1000,10 @@ static void xhci_ring_init(XHCIState *xhci, XHCIRing *ring,
static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
dma_addr_t *addr)
{
+ uint16_t trbcnt = 0;
PCIDevice *pci_dev = PCI_DEVICE(xhci);
- while (1) {
+ while (trbcnt++ < TRB_MAX) {
TRBType type;
pci_dma_read(pci_dev, ring->dequeue, trb, TRB_SIZE);
trb->addr = ring->dequeue;
@@ -1032,6 +1034,8 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
}
}
}
+
+ return 0;
}
static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
--
2.5.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] usb: xHCI: add check to limit command TRB processing
2016-10-06 5:50 [Qemu-devel] [PATCH] usb: xHCI: add check to limit command TRB processing P J P
@ 2016-10-07 8:22 ` Gerd Hoffmann
2016-10-07 11:40 ` P J P
0 siblings, 1 reply; 3+ messages in thread
From: Gerd Hoffmann @ 2016-10-07 8:22 UTC (permalink / raw)
To: P J P; +Cc: Qemu Developers, Li Qiang, Prasad J Pandit
[-- Attachment #1: Type: text/plain, Size: 887 bytes --]
On Do, 2016-10-06 at 11:20 +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> USB xHCI controller uses ring of Transfer Request Blocks(TRB)
> to process USB commands. These are processed by loop in
> 'xhci_ring_fetch'. A guest user could make it read and process
> a same TRB infinitely. Limit number of command TRBs to avoid it.
I think it is better to apply the limit to link trbs only (which allow
to jump to another address so the guest can build loops with it). Also
I think the limit can be much stricter then without breaking stuff as
typically a link trb is used at the end of a page full of normal trbs,
to jump to the next page with trbs. And we have the same problem in
both xhci_ring_fetch and xhci_ring_chain_length, so we should fix both.
Is there a reproducer? If so, can you try the attached patch with it?
thanks,
Gerd
[-- Attachment #2: 0001-xhci-limit-the-number-of-link-trbs-we-are-willing-to.patch --]
[-- Type: text/x-patch, Size: 2099 bytes --]
From 20009bdaf95d10bf748fa69b104672d3cfaceddf Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 7 Oct 2016 10:15:29 +0200
Subject: [PATCH] xhci: limit the number of link trbs we are willing to process
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/usb/hcd-xhci.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 726435c..ee4fa48 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -54,6 +54,8 @@
* to the specs when it gets them */
#define ER_FULL_HACK
+#define TRB_LINK_LIMIT 4
+
#define LEN_CAP 0x40
#define LEN_OPER (0x400 + 0x10 * MAXPORTS)
#define LEN_RUNTIME ((MAXINTRS + 1) * 0x20)
@@ -1000,6 +1002,7 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
dma_addr_t *addr)
{
PCIDevice *pci_dev = PCI_DEVICE(xhci);
+ uint32_t link_cnt = 0;
while (1) {
TRBType type;
@@ -1026,6 +1029,9 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
ring->dequeue += TRB_SIZE;
return type;
} else {
+ if (++link_cnt > TRB_LINK_LIMIT) {
+ return 0;
+ }
ring->dequeue = xhci_mask64(trb->parameter);
if (trb->control & TRB_LK_TC) {
ring->ccs = !ring->ccs;
@@ -1043,6 +1049,7 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
bool ccs = ring->ccs;
/* hack to bundle together the two/three TDs that make a setup transfer */
bool control_td_set = 0;
+ uint32_t link_cnt = 0;
while (1) {
TRBType type;
@@ -1058,6 +1065,9 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
type = TRB_TYPE(trb);
if (type == TR_LINK) {
+ if (++link_cnt > TRB_LINK_LIMIT) {
+ return -length;
+ }
dequeue = xhci_mask64(trb.parameter);
if (trb.control & TRB_LK_TC) {
ccs = !ccs;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] usb: xHCI: add check to limit command TRB processing
2016-10-07 8:22 ` Gerd Hoffmann
@ 2016-10-07 11:40 ` P J P
0 siblings, 0 replies; 3+ messages in thread
From: P J P @ 2016-10-07 11:40 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Qemu Developers, Li Qiang
Hello Gerd,
+-- On Fri, 7 Oct 2016, Gerd Hoffmann wrote --+
| I think it is better to apply the limit to link trbs only (which allow
| to jump to another address so the guest can build loops with it). Also
| I think the limit can be much stricter then without breaking stuff as
| typically a link trb is used at the end of a page full of normal trbs,
| to jump to the next page with trbs.
Okay.
| both xhci_ring_fetch and xhci_ring_chain_length, so we should fix both.
| Is there a reproducer? If so, can you try the attached patch with it?
Yes, the attached patch does fix this issue.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-10-07 11:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-06 5:50 [Qemu-devel] [PATCH] usb: xHCI: add check to limit command TRB processing P J P
2016-10-07 8:22 ` Gerd Hoffmann
2016-10-07 11:40 ` P J P
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.