On Fri, Jan 29, 2021 at 08:16:27AM +0100, Anton Lindqvist wrote: > Hi, > While running usbhidctl on my USB mouse it occasionally fails as > follows: > > # usbhidctl -f /dev/wsmouse2 > usbhidctl: USB_GET_REPORT (probably not supported by device): Bad > address > > The EFAULT happens during copyin(9) in sys_ioctl() while copying the > supplied usb_ctl_report structure which is declared as follows: > > struct usb_ctl_report { > int ucr_report; > u_char ucr_data[1024]; /* filled data size will vary */ > }; > > ... and the corresponding ioctl command: > > #define USB_GET_REPORT _IOWR('U', 23, struct usb_ctl_report) > > usbhidctl tries to be memory efficient by only allocating a buffer big > enough to hold the requested report. Such report is often smaller than > 1024 bytes. > > However, the kernel will always copy `sizeof(struct usb_ctl_report)' > bytes from the address passed from user space. I would assume the EFAULT > happens when `addr + sizeof(struct usb_ctl_report)' crosses a page > boundary and the adjacent page is not mapped. Unconditionally allocating > the correct size fixes the problem. > > Comments? OK?
Makes sense to me. ok mglocker@ > Index: usbhid.c > =================================================================== > RCS file: /cvs/src/usr.bin/usbhidctl/usbhid.c,v > retrieving revision 1.15 > diff -u -p -r1.15 usbhid.c > --- usbhid.c 28 Jun 2019 13:35:05 -0000 1.15 > +++ usbhid.c 28 Jan 2021 20:27:17 -0000 > @@ -394,13 +394,7 @@ allocreport(struct Sreport *report, repo > report->size = reptsize; > > if (report->size > 0) { > - /* > - * Allocate a buffer with enough space for the > - * report in the variable-sized data field. > - */ > - report->buffer = malloc(sizeof(*report->buffer) - > - sizeof(report->buffer->ucr_data) + > - report->size); > + report->buffer = malloc(sizeof(*report->buffer)); > if (report->buffer == NULL) > err(1, NULL); > } else >