Re: [PATCH] usb: gadget: aspeed: fix buffer overflow
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2022-10-25 00:32:12
Also in:
linux-aspeed, linux-usb, lkml
On Mon, 2022-10-24 at 09:48 +0000, Lei YU wrote:
From: Henry Tian <redacted>
I wrote that driver, please CC me on further patches to it (thanks Joel for the heads up).
In ast_vhub_epn_handle_ack() when the received data length exceeds the buffer, it does not check the case and just copies to req.buf and cause a buffer overflow, kernel oops on this case.
.../... Thanks ! Seems like a legit bug, however:
quoted hunk ↗ jump to hunk
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c index b5252880b389..56e55472daa1 100644 --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c@@ -84,6 +84,7 @@ static void ast_vhub_epn_handle_ack(struct ast_vhub_ep *ep){ struct ast_vhub_req *req; unsigned int len; + int status = 0; u32 stat; /* Read EP status */@@ -119,9 +120,15 @@ static void ast_vhub_epn_handle_ack(struct ast_vhub_ep *ep)len = VHUB_EP_DMA_TX_SIZE(stat); /* If not using DMA, copy data out if needed */ - if (!req->req.dma && !ep->epn.is_in && len) - memcpy(req->req.buf + req->req.actual, ep->buf, len); - + if (!req->req.dma && !ep->epn.is_in && len) { + if (req->req.actual + len > req->req.length) { + req->last_desc = 1; + status = -EOVERFLOW; + goto done;
Should we stall as well ? Should we continue receiving and just dropping the data until we have a small packet ? Otherwise the EP could get out of sync for subsequent ones... Additionally, I'm curious, why in this specific case is the device sending more data than the buffer can hold ? The MTU change should have resulted in buffers being re-allocated no ? Or did you change the MTU on the remote and not on the local device ? Cheers, Ben. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel