P J P <ppan...@redhat.com> 于2020年9月11日周五 下午8:30写道: > > From: Prasad J Pandit <p...@fedoraproject.org> > > While servicing the OHCI transfer descriptors(TD), OHCI host > controller derives variables 'start_addr', 'end_addr', 'len' > etc. from values supplied by the host controller driver. > Host controller driver may supply values such that using > above variables leads to out-of-bounds access or loop issues. > Add checks to avoid them. > > AddressSanitizer: stack-buffer-overflow on address 0x7ffd53af76a0 > READ of size 2 at 0x7ffd53af76a0 thread T0 > #0 ohci_service_iso_td ../hw/usb/hcd-ohci.c:734 > #1 ohci_service_ed_list ../hw/usb/hcd-ohci.c:1180 > #2 ohci_process_lists ../hw/usb/hcd-ohci.c:1214 > #3 ohci_frame_boundary ../hw/usb/hcd-ohci.c:1257 > #4 timerlist_run_timers ../util/qemu-timer.c:572 > #5 qemu_clock_run_timers ../util/qemu-timer.c:586 > #6 qemu_clock_run_all_timers ../util/qemu-timer.c:672 > #7 main_loop_wait ../util/main-loop.c:527 > #8 qemu_main_loop ../softmmu/vl.c:1676 > #9 main ../softmmu/main.c:50 >
Hello Prasad, Could you also provide the reproducer? > Reported-by: Gaoning Pan <p...@zju.edu.cn> > Reported-by: Yongkang Jia <j_kan...@163.com> > Reported-by: Yi Ren <yunye...@alibaba-inc.com> > Signed-off-by: Prasad J Pandit <p...@fedoraproject.org> > --- > hw/usb/hcd-ohci.c | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > index 1e6e85e86a..76fb9282e3 100644 > --- a/hw/usb/hcd-ohci.c > +++ b/hw/usb/hcd-ohci.c > @@ -691,6 +691,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct > ohci_ed *ed, > the next ISO TD of the same ED */ > > trace_usb_ohci_iso_td_relative_frame_number_big(relative_frame_number, > frame_count); > + if (OHCI_CC_DATAOVERRUN == OHCI_BM(iso_td.flags, TD_CC)) { > + /* avoid infinite loop */ > + return 1; > + } I think it is better to split this patch to 2 or three as the infinite loop as the buffer overflow are independent. 1. here the infinite loop > + > OHCI_SET_BM(iso_td.flags, TD_CC, OHCI_CC_DATAOVERRUN); > ed->head &= ~OHCI_DPTR_MASK; > ed->head |= (iso_td.next & OHCI_DPTR_MASK); > @@ -731,7 +736,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct > ohci_ed *ed, > } > > start_offset = iso_td.offset[relative_frame_number]; > - next_offset = iso_td.offset[relative_frame_number + 1]; > + if (relative_frame_number < frame_count) { > + next_offset = iso_td.offset[relative_frame_number + 1]; > + } else { > + next_offset = iso_td.be; > + } 2. here the stack buffer overflow > > if (!(OHCI_BM(start_offset, TD_PSW_CC) & 0xe) || > ((relative_frame_number < frame_count) && > @@ -764,7 +773,12 @@ static int ohci_service_iso_td(OHCIState *ohci, struct > ohci_ed *ed, > } > } else { > /* Last packet in the ISO TD */ > - end_addr = iso_td.be; > + end_addr = next_offset; > + } > + > + if (start_addr > end_addr) { > + trace_usb_ohci_iso_td_bad_cc_overrun(start_addr, end_addr); > + return 1; > } > > if ((start_addr & OHCI_PAGE_MASK) != (end_addr & OHCI_PAGE_MASK)) { > @@ -773,6 +787,9 @@ static int ohci_service_iso_td(OHCIState *ohci, struct > ohci_ed *ed, > } else { > len = end_addr - start_addr + 1; > } > + if (len > sizeof(ohci->usb_buf)) { > + len = sizeof(ohci->usb_buf); > + } > > if (len && dir != OHCI_TD_DIR_IN) { > if (ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, > @@ -975,8 +992,16 @@ static int ohci_service_td(OHCIState *ohci, struct > ohci_ed *ed) > if ((td.cbp & 0xfffff000) != (td.be & 0xfffff000)) { > len = (td.be & 0xfff) + 0x1001 - (td.cbp & 0xfff); > } else { > + if (td.cbp > td.be) { > + trace_usb_ohci_iso_td_bad_cc_overrun(td.cbp, td.be); > + ohci_die(ohci); > + return 1; > + } > len = (td.be - td.cbp) + 1; > } > + if (len > sizeof(ohci->usb_buf)) { > + len = sizeof(ohci->usb_buf); > + } > 3. Then here is the heap overflow. So I think it can be more easier to review to split this to 3 patches. Thanks, Li Qiang > pktlen = len; > if (len && dir != OHCI_TD_DIR_IN) { > -- > 2.26.2 > >