pkg/50365: libusb in pkg had a flaw

[dtyson%anduin.org.uk@localhost]

>Number:         50365
>Category:       pkg
>Synopsis:       libusb usb_interrupt_read routine truncates data
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    pkg-manager
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Oct 24 20:10:00 +0000 2015
>Originator:     Dave Tyson
>Release:        NetBSD 6.99.47
>Organization:
	Anduin
>Environment:
	
	
System: NetBSD cruncher.anduin.org.uk 6.99.47 NetBSD 6.99.47 (GENERIC) #0: Mon Jan 5 16:58:56 GMT 2015 root%cruncher.anduin.org.uk@localhost:/usr/obj/sys/arch/amd64/compile/GENERIC amd64
Architecture: x86_64
Machine: amd64
>Description:
The version of libusb in pkgsrc has a flaw in the routine which handles 
the reading of data from a usb device using the usb_interrupt_read 
routine.

Basically the routine sets a timeout and elects to permit short transfers. 
It then enters a loop reading from the endpoint until the requested number
of bytes have been transferred. Because the device may not be able to 
transfer all the data in a single read, the code keeps track of the number
of bytes it has read so it can change the size of the request as needed. The
loop doing the read has an incorrectly specified termination condition which
means that the read loop can terminate early before all the data has been 
transferred. The flaw can be seen in the snippet of code below which is taken
from the source code after the pkg patches have been applied:


int usb_interrupt_read(usb_dev_handle *dev, int ep, char *bytes, int size,
                       int timeout)
{
  int fd, ret, retrieved = 0, one = 1, requested;
 
/* .... irrelevent stuff removed .... */

  do {
    requested = size - retrieved;
    ret = read(fd, bytes+retrieved, requested);
    if (ret < 0)
#ifdef FreeBSD_like_device_names
      USB_ERROR_STR(-errno, "error reading from interrupt endpoint %s.%d: %s",
                    dev->device->filename, UE_GET_ADDR(ep), strerror(errno));
#else
      USB_ERROR_STR(-errno, "error reading from interrupt endpoint %s.%02d: %s",
                  dev->device->filename, UE_GET_ADDR(ep), strerror(errno));
#endif
    retrieved += ret;
  } while (ret > 0 && retrieved < size && ret == requested);

  return retrieved;
}


If the usb device returns a smaller amount of data than requested then the final
condition 'ret == requested' is false and so the loop terminates rather than
repeating to read more data. I think the condition should read 'ret != requested'
and with that change the loop operates correctly.

This was found when trying to read 32 bytes of data from a device which just 
returned data in 8 byte chunks...

I am amazed no-one else has been hit by this in the past 11 years!
>How-To-Repeat:
Try and read data using usb_interrupt_read and make sure the request asks for
more data than the device can send in one usb transfer. Notice you only get part 
of the requested block. 
>Fix:
This requires a simple change to patch-ad below and a regen of the patch checksums.

--- patch-ad.orig       2015-10-24 19:58:19.000000000 +0100
+++ patch-ad    2015-10-24 19:59:06.000000000 +0100
@@ -141,7 +141,7 @@
  #endif
      retrieved += ret;
 -  } while (ret > 0 && retrieved < size);
-+  } while (ret > 0 && retrieved < size && ret == requested);                                                                                    
++  } while (ret > 0 && retrieved < size && ret != requested);                                                                                    
                                                                                                                                                  
    return retrieved;                                                                                                                             
  }


The updated distinfo file will read:

SHA1 (patch-ad) = 54a82ce27b09e5abb61200cf77a4f8e6e00b586d

>Unformatted: