[3/8] usb: fix unbounded stack for ohci_td_pkt

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/usb/hcd-ohci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

On 08/03/2016 08:00, Peter Xu wrote:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/usb/hcd-ohci.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 17ed461..c3cd4e2 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -936,11 +936,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
>  #ifdef trace_event_get_state
>  static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
>  {
> +#define __TEMP_WIDTH (16)
>      bool print16 = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_SHORT);
>      bool printall = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_FULL);
> -    const int width = 16;
>      int i;
> -    char tmp[3 * width + 1];
> +    char tmp[3 * __TEMP_WIDTH + 1];
>      char *p = tmp;
>  
>      if (!printall && !print16) {
> @@ -948,7 +948,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
>      }
>  
>      for (i = 0; ; i++) {
> -        if (i && (!(i % width) || (i == len))) {
> +        if (i && (!(i % __TEMP_WIDTH) || (i == len))) {
>              if (!printall) {
>                  trace_usb_ohci_td_pkt_short(msg, tmp);
>                  break;
> @@ -963,6 +963,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
>  
>          p += sprintf(p, " %.2x", buf[i]);
>      }
> +#undef __TEMP_WIDTH
>  }
>  #else
>  static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> 

This is a compiler false positive.  You can change "i % width" to

   p - tmp == ARRAY_SIZE(tmp) - 1

if you want to avoid it, but I'd just ignore this one.

Paolo

On Tue, Mar 08, 2016 at 01:20:45PM +0100, Paolo Bonzini wrote:
> 
> 
> On 08/03/2016 08:00, Peter Xu wrote:
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/usb/hcd-ohci.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> > index 17ed461..c3cd4e2 100644
> > --- a/hw/usb/hcd-ohci.c
> > +++ b/hw/usb/hcd-ohci.c
> > @@ -936,11 +936,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
> >  #ifdef trace_event_get_state
> >  static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> >  {
> > +#define __TEMP_WIDTH (16)
> >      bool print16 = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_SHORT);
> >      bool printall = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_FULL);
> > -    const int width = 16;
> >      int i;
> > -    char tmp[3 * width + 1];
> > +    char tmp[3 * __TEMP_WIDTH + 1];
> >      char *p = tmp;
> >  
> >      if (!printall && !print16) {
> > @@ -948,7 +948,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> >      }
> >  
> >      for (i = 0; ; i++) {
> > -        if (i && (!(i % width) || (i == len))) {
> > +        if (i && (!(i % __TEMP_WIDTH) || (i == len))) {
> >              if (!printall) {
> >                  trace_usb_ohci_td_pkt_short(msg, tmp);
> >                  break;
> > @@ -963,6 +963,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> >  
> >          p += sprintf(p, " %.2x", buf[i]);
> >      }
> > +#undef __TEMP_WIDTH
> >  }
> >  #else
> >  static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> > 
> 
> This is a compiler false positive.  You can change "i % width" to
> 
>    p - tmp == ARRAY_SIZE(tmp) - 1
> 
> if you want to avoid it, but I'd just ignore this one.

Then I'd like to drop this patch for now.

Btw, do you know why the compiler got this false positive? Since we
declared width as constant. Is it a... "bug" or a "feature"?

Thanks.
Peter