On Thu, Dec 21, 2023, Krishna Kurapati wrote: > In the current implementation, the check_event_buf call reads the > GEVNTCOUNT register to determine the amount of event data generated > and copies it from ev->buf to ev->cache after masking interrupt. > > During copy if the amount of data to be copied is more than > (length - lpos), we fill the ev->cache till the end of 4096 byte > buffer allocated and then start filling from the top (lpos = 0). > > In one instance of SMMU crash it is observed that GEVNTCOUNT register > reads more than 4096 bytes: > > dwc3_readl base=0xffffffc0091dc000 offset=50188 value=63488 > > (offset = 50188 -> 0xC40C) -> reads 63488 bytes > > As per crash dump: > dwc->lpos = 2056 > > and evt->buf is at 0xFFFFFFC009185000 and the crash is at > 0xFFFFFFC009186000. The diff which is exactly 0x1000 bytes. > > We first memcpy 2040 bytes from (buf + lpos) to (buf + 0x1000). > > And then we copy the rest of the data (64388 - 2040) from beginning > of dwc->ev_buf. While doing so we go beyond bounds as we are trying > to memcpy 62348 bytes into a 4K buffer resulting in crash. > > Fix this by ignoring the interrupt when GEVNTCOUNT register reads a > value more than the event ring allocated. > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx> > --- > Changes in v2: > Instead of fixing amount of data being copied from ring, ignored > the interrupt when count is corrupt as per suggestion from Thinh. > > Link to v1: > https://urldefense.com/v3/__https://lore.kernel.org/all/20230521100330.22478-1-quic_kriskura@xxxxxxxxxxx/__;!!A4F2R9G_pg!ewM3u9Pdk8yD9eU24sOuQqmhm8M2VpGXH8zALqVWGKffGbcJxrtyKKlUPuh8SS2arqO09DjnC9atC3bemEpx-g5UQMllbA$ > > drivers/usb/dwc3/gadget.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 858fe4c299b7..e27933fdcce3 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -4451,6 +4451,7 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void *_evt) > static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) > { > struct dwc3 *dwc = evt->dwc; > + int ret = IRQ_WAKE_THREAD; irqreturn_t instead of int? > u32 amount; > u32 count; > > @@ -4480,6 +4481,12 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) > if (!count) > return IRQ_NONE; > > + if (count > evt->length) { > + dev_err(dwc->dev, "GEVTCOUNT corrupt\n"); > + ret = IRQ_NONE; > + goto done; > + } > + > evt->count = count; > evt->flags |= DWC3_EVENT_PENDING; > > @@ -4493,9 +4500,10 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) > if (amount < count) > memcpy(evt->cache, evt->buf, count - amount); > > +done: > dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); Don't update the GEVNTCOUNT if the read is invalid. We're not processing any event, so we should not write back the "count" that we did not process. > > - return IRQ_WAKE_THREAD; > + return ret; > } > > static irqreturn_t dwc3_interrupt(int irq, void *_evt) > -- > 2.42.0 > Thanks, Thinh
