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 - 1.15
> +++ usbhid.c 28 Jan 2021 20:27:17 -
> @@ -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
>
