[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    usbhidctl: efault
From:       Anton Lindqvist <anton () openbsd ! org>
Date:       2021-01-29 7:16:27
Message-ID: YBO2S56qAd8d5owA () desk ! basename ! se
[Download RAW message or body]

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?

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

[prev in list] [next in list] [prev in thread] [next in thread]