Re: [PATCH] hw: usb: hcd-ohci: check len and frame_number variables
+-- On Fri, 11 Sep 2020, Alexander Bulekov wrote --+ | > On 200911 2257, Li Qiang wrote: | > > Could you also provide the reproducer? * Sorry, we can not share reproducers on the list, I'm afraid. * Thank you Alex for the -qtests. | > > 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 | > > 2. here the stack buffer overflow | > > 3. Then here is the heap overflow. | > > | > > So I think it can be more easier to review to split this to 3 patches. * These issues are in the same UHCI controller and share the same pattern, triggered while processing ED/TD lists. They can be combined as a single CVE. * Infinite-loop is different. I'll break it into two patches, one for OOB access and one to avoid an infinite loop case. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] hw: usb: hcd-ohci: check len and frame_number variables
On 200911 1520, Alexander Bulekov wrote:
> On 200911 2257, Li Qiang wrote:
> > P J P 于2020年9月11日周五 下午8:30写道：
> > >
> > > From: Prasad J Pandit
> > >
> > > 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
> > > Reported-by: Yongkang Jia
> > > Reported-by: Yi Ren
> > > Signed-off-by: Prasad J Pandit
> > > ---
> > > 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
... and for the stack overflow:
cat << EOF | ./qemu-system-i386 -device pci-ohci,id=usb \
-nodefaults -qtest stdio -nographic
outl 0xcf8 0x80001013
outl 0xcfc 0xfdff5955
outl 0xcf8 0x80001002
outl 0xcfc 0xd3073d2f
writeq 0x5500 0x5a55b984d3fd0200
clock_step
clock_step
clock_step
clock_step
clock_step
clock_step
write 0x18 0x1 0x7a
write 0x19 0x1 0xab
write 0xab7b 0x1 0xab
write 0xab7f 0x1 0xff
write 0xab82 0x1 0x7a
write 0xab83 0x1 0xab
write 0xab70 0x1 0xff
write 0xab71 0x1 0xff
write 0xab73 0x1 0xff
write 0xab75 0x1 0xab
clock_step
EOF
> > >
> > > 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 & 0xf000) != (td.be & 0xf000)) {
> > > len = (td.be & 0xfff) + 0x1001 - (td.cbp & 0xfff);
> > > } else {
> > > +if (td.cbp > td.be) {
> > > +trace_usb_ohci_iso_td_bad_cc_overrun(td.cbp
Re: [PATCH] hw: usb: hcd-ohci: check len and frame_number variables
On 200911 2257, Li Qiang wrote:
> P J P 于2020年9月11日周五 下午8:30写道：
> >
> > From: Prasad J Pandit
> >
> > 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
> > Reported-by: Yongkang Jia
> > Reported-by: Yi Ren
> > Signed-off-by: Prasad J Pandit
> > ---
> > 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 & 0xf000) != (td.be & 0xf000)) {
> > 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.
>
Maybe this is the heap-overflow?
cat << EOF | ./qemu-system-i386 -device pci-ohci,id=usbb \
-device usb-tablet,bus=usbb.0,port=1,usb_version=1 \
-nodefaults -qtest stdio -nographic
outl 0xcf8 0x80001013
outl 0xcfc 0x1fd555a
outl 0xcf8 0x80001002
outl 0xcfc 0x7fe072f
write 0x5a04 0x1 0xa5
write 0x0 0x1 0x26
write 0x1 0x1 0xfc
write 0xfc27 0x1 0xaa
write 0xfc30 0x1 0x04
write 0x40006 0x1 0x27
write 0x4000e 0x1 0x27
write 0x40011 0x1 0xff
EOF
==3325498==ERROR: AddressSanitizer: heap-buffer
Re: [PATCH] hw: usb: hcd-ohci: check len and frame_number variables
P J P 于2020年9月11日周五 下午8:30写道：
>
> From: Prasad J Pandit
>
> 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
> Reported-by: Yongkang Jia
> Reported-by: Yi Ren
> Signed-off-by: Prasad J Pandit
> ---
> 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 & 0xf000) != (td.be & 0xf000)) {
> 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
>
>
