[PATCH] hw/usb/hcd-xhci: Avoid variable-length array in xhci_get_port_bandwidth()

[PATCH] hw/usb/hcd-xhci: Avoid variable-length array in xhci_get_port_bandwidth()

[Peter Maydell]

In xhci_get_port_bandwidth(), we use a variable-length array to
construct the buffer to send back to the guest. Avoid the VLA
by using dma_memory_set() to directly request the memory system
to fill the guest memory with a string of '80's.

The codebase has very few VLAs, and if we can get rid of them all we
can make the compiler error on new additions.  This is a defensive
measure against security bugs where an on-stack dynamic allocation
isn't correctly size-checked (e.g.  CVE-2021-3527).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Use of dma_memory_set() is a suggestion from RTH from Philippe's
original attempt.  If we ever do anything about the "use real
values" TODO we'll need to do something else (eg heap-allocated
array), but since we haven't done so since the code was written
in 2012 it doesn't seem very likely we'll ever do so.
---
 hw/usb/hcd-xhci.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index b89b618ec21..324177ad5df 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2434,7 +2434,6 @@ static void xhci_detach_slot(XHCIState *xhci, USBPort *uport)
 static TRBCCode xhci_get_port_bandwidth(XHCIState *xhci, uint64_t pctx)
 {
     dma_addr_t ctx;
-    uint8_t bw_ctx[xhci->numports+1];
 
     DPRINTF("xhci_get_port_bandwidth()\n");
 
@@ -2442,11 +2441,10 @@ static TRBCCode xhci_get_port_bandwidth(XHCIState *xhci, uint64_t pctx)
 
     DPRINTF("xhci: bandwidth context at "DMA_ADDR_FMT"\n", ctx);
 
-    /* TODO: actually implement real values here */
-    bw_ctx[0] = 0;
-    memset(&bw_ctx[1], 80, xhci->numports); /* 80% */
-    if (dma_memory_write(xhci->as, ctx, bw_ctx, sizeof(bw_ctx),
-                     MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+    /* TODO: actually implement real values here. This is 80% for all ports. */
+    if (stb_dma(xhci->as, ctx, 0, MEMTXATTRS_UNSPECIFIED) != MEMTX_OK ||
+        dma_memory_set(xhci->as, ctx + 1, 80, xhci->numports,
+                       MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory write failed!\n",
                       __func__);
         return CC_TRB_ERROR;
-- 
2.34.1

Re: [PATCH] hw/usb/hcd-xhci: Avoid variable-length array in xhci_get_port_bandwidth()

[Philippe Mathieu-Daudé]

On 24/8/23 18:48, Peter Maydell wrote:
> In xhci_get_port_bandwidth(), we use a variable-length array to
> construct the buffer to send back to the guest. Avoid the VLA
> by using dma_memory_set() to directly request the memory system
> to fill the guest memory with a string of '80's.
> 
> The codebase has very few VLAs, and if we can get rid of them all we
> can make the compiler error on new additions.  This is a defensive
> measure against security bugs where an on-stack dynamic allocation
> isn't correctly size-checked (e.g.  CVE-2021-3527).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Use of dma_memory_set() is a suggestion from RTH from Philippe's
> original attempt.  If we ever do anything about the "use real
> values" TODO we'll need to do something else (eg heap-allocated
> array), but since we haven't done so since the code was written
> in 2012 it doesn't seem very likely we'll ever do so.
> ---
>   hw/usb/hcd-xhci.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)

Thanks!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>