Changing permissions of /dev/usb[n] to 664?

[Craig Rodrigues]

On Sun, Nov 07, 2004 at 12:58:14PM -0700, M. Warner Losh wrote:
> This looks good, but we should audit all the ioctls to make sure the
> ones that modify anything have the proper checks to make sure the fd
> was opened for write.

OK.  Here is another iteration of the patch.
It does the following:
- opens /dev/usb[n] as 664
- puts suser() permission checks in the following paths:
    USB_REQUEST ioctl()
    usbpoll()
    usbread()

This is what a non-root user can and cannot do on /dev/usb[n]: 

Allowed
=======
USB_DISCOVER
USB_DEVICEINFO
USB_DEVICESTATS
usbopen()
usbclose()

Forbidden
=========
USB_REQUEST
usbread()
usbpoll()

The result of this patch is that a non-root user can
run usbdevs without a problem.

I also have a small test program where I tried running
different ioctl's as non-root and this is the output I got:

Executing ioctl(): USB_REQUEST Operation not permitted
Executing ioctl(): USB_DISCOVER...OK
Executing ioctl(): USB_DEVICEINFO...OK
Executing ioctl(): USB_DEVICESTATS...OK

Comments?

-- 
Craig Rodrigues        
rodrigc at crodrigues.org
-------------- next part --------------
--- usb.c.orig	Mon Nov 29 23:27:20 2004
+++ usb.c	Tue Nov 30 00:04:00 2004
@@ -320,11 +320,11 @@
 	/* The per controller devices (used for usb_discover) */
 	/* XXX This is redundant now, but old usbd's will want it */
 	sc->sc_usbdev = make_dev(&usb_cdevsw, device_get_unit(self), UID_ROOT,
-	    GID_OPERATOR, 0660, "usb%d", device_get_unit(self));
+	    GID_OPERATOR, 0664, "usb%d", device_get_unit(self));
 	if (usb_ndevs++ == 0) {
 		/* The device spitting out events */
 		usb_dev = make_dev(&usb_cdevsw, USB_DEV_MINOR, UID_ROOT,
-		    GID_OPERATOR, 0660, "usb");
+		    GID_OPERATOR, 0664, "usb");
 	}
 #endif
 
@@ -515,13 +515,16 @@
 	int unit = USBUNIT(dev);
 	int s, error, n;
 
+	error = suser(curthread);
+	if (error)
+		return error;
+
 	if (unit != USB_DEV_MINOR)
 		return (ENODEV);
 
 	if (uio->uio_resid != sizeof(struct usb_event))
 		return (EINVAL);
 
-	error = 0;
 	s = splusb();
 	for (;;) {
 		n = usb_get_next_event(&ue);
@@ -605,6 +608,10 @@
 		usbd_status err;
 		int error = 0;
 
+		error = suser(p);
+		if (error)
+			return error;
+
 		DPRINTF(("usbioctl: USB_REQUEST addr=%d len=%d\n", addr, len));
 		if (len < 0 || len > 32768)
 			return (EINVAL);
@@ -680,6 +687,10 @@
 {
 	int revents, mask, s;
 	int unit = USBUNIT(dev);
+	int error = suser(p);
+
+	if (error)
+		return error;
 
 	if (unit == USB_DEV_MINOR) {
 		revents = 0;
-------------- next part --------------
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <dev/usb/usb.h>
#include <errno.h>

void
execute_ioctl(int fd, int cmd, void *arg, const char *name)
{
	int e;
	printf("Executing ioctl(): %s", name);
	e = ioctl(fd, cmd, arg);
	if (e == -1 ) {
		printf(" %s\n", strerror(errno));
	} else {
		printf("...OK\n");
	}
}

int
main(int argc, char *argv[])
{
	struct usb_ctl_request ucr;
	struct usb_device_stats uds;
	struct usb_device_info udi;
	int fd = open("/dev/usb1", O_RDONLY);

	if (fd == -1 ) {
		perror(":");
		exit(1);
 	 }
	ucr.ucr_request.wLength[0] = 0x00;
	ucr.ucr_request.wLength[1] = 0x80;

	udi.udi_addr = 1;

	execute_ioctl(fd, USB_REQUEST, &ucr, "USB_REQUEST");
	execute_ioctl(fd, USB_DISCOVER, NULL, "USB_DISCOVER");
	execute_ioctl(fd, USB_DEVICEINFO, &udi, "USB_DEVICEINFO");
	execute_ioctl(fd, USB_DEVICESTATS, &uds, "USB_DEVICESTATS");

	close(fd); 

	return 0;
}