Bug 116181
| Summary: | /dev/io-related io access permissions are not propagated to every thread in a process | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Kyle Larose <kmlarose> |
| Component: | threads | Assignee: | attilio |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | ||
| Priority: | Normal | ||
| Version: | 6.2-RELEASE | ||
| Hardware: | Any | ||
| OS: | Any | ||
It should be noted that I tested the example app with -lthr (so it *could* be a bug with the threading library, though it still sounds more like a problem with the kernel to me). -- Kyle Larose Vice President, University of Waterloo Computer Science Club. Responsible Changed From-To: freebsd-bugs->freebsd-threads Reclassify. Author: attilio Date: Wed Apr 28 15:38:01 2010 New Revision: 207329 URL: http://svn.freebsd.org/changeset/base/207329 Log: - Extract the IODEV_PIO interface from ia64 and make it MI. In the end, it does help fixing /dev/io usage from multithreaded processes. - On i386 and amd64 the old behaviour is kept but multithreaded processes must use the new interface in order to work well. - Support for the other architectures is greatly improved, where necessary, by the necessity to define very small things now. Manpage update will happen shortly. Sponsored by: Sandvine Incorporated PR: threads/116181 Reviewed by: emaste, marcel MFC after: 3 weeks Added: head/sys/dev/io/iodev.h (contents, props changed) Modified: head/sys/amd64/amd64/io.c head/sys/amd64/include/iodev.h head/sys/dev/io/iodev.c head/sys/i386/i386/io.c head/sys/i386/include/iodev.h head/sys/ia64/ia64/iodev_machdep.c head/sys/ia64/include/iodev.h Modified: head/sys/amd64/amd64/io.c ============================================================================== --- head/sys/amd64/amd64/io.c Wed Apr 28 15:15:06 2010 (r207328) +++ head/sys/amd64/amd64/io.c Wed Apr 28 15:38:01 2010 (r207329) @@ -28,60 +28,32 @@ __FBSDID("$FreeBSD$"); #include <sys/param.h> -#include <sys/conf.h> -#include <sys/fcntl.h> -#include <sys/lock.h> -#include <sys/malloc.h> -#include <sys/mutex.h> -#include <sys/priv.h> #include <sys/proc.h> -#include <sys/signalvar.h> -#include <sys/systm.h> -#include <machine/db_machdep.h> #include <machine/frame.h> -#include <machine/psl.h> -#include <machine/specialreg.h> - -#include <vm/vm.h> -#include <vm/pmap.h> - #include <machine/iodev.h> +#include <machine/psl.h> -/* ARGSUSED */ int -ioopen(struct cdev *dev __unused, int flags __unused, int fmt __unused, - struct thread *td) +iodev_open(struct thread *td) { - int error; - - error = priv_check(td, PRIV_IO); - if (error != 0) - return (error); - error = securelevel_gt(td->td_ucred, 0); - if (error != 0) - return (error); td->td_frame->tf_rflags |= PSL_IOPL; - return (0); } -/* ARGSUSED */ int -ioclose(struct cdev *dev __unused, int flags __unused, int fmt __unused, - struct thread *td) +iodev_close(struct thread *td) { - td->td_frame->tf_rflags &= ~PSL_IOPL; + td->td_frame->tf_rflags &= ~PSL_IOPL; return (0); } /* ARGSUSED */ int -ioioctl(struct cdev *dev __unused, u_long cmd __unused, caddr_t data __unused, - int fflag __unused, struct thread *td __unused) +iodev_ioctl(u_long cmd __unused, caddr_t data __unused) { - return (ENXIO); + return (ENOIOCTL); } Modified: head/sys/amd64/include/iodev.h ============================================================================== --- head/sys/amd64/include/iodev.h Wed Apr 28 15:15:06 2010 (r207328) +++ head/sys/amd64/include/iodev.h Wed Apr 28 15:38:01 2010 (r207329) @@ -25,7 +25,22 @@ * * $FreeBSD$ */ +#ifndef _MACHINE_IODEV_H_ +#define _MACHINE_IODEV_H_ -d_open_t ioopen; -d_close_t ioclose; -d_ioctl_t ioioctl; +#ifdef _KERNEL +#include <machine/cpufunc.h> + +#define iodev_read_1 inb +#define iodev_read_2 inw +#define iodev_read_4 inl +#define iodev_write_1 outb +#define iodev_write_2 outw +#define iodev_write_4 outl + +int iodev_open(struct thread *td); +int iodev_close(struct thread *td); +int iodev_ioctl(u_long cmd, caddr_t data); + +#endif /* _KERNEL */ +#endif /* _MACHINE_IODEV_H_ */ Modified: head/sys/dev/io/iodev.c ============================================================================== --- head/sys/dev/io/iodev.c Wed Apr 28 15:15:06 2010 (r207328) +++ head/sys/dev/io/iodev.c Wed Apr 28 15:38:01 2010 (r207329) @@ -30,22 +30,27 @@ __FBSDID("$FreeBSD$"); #include <sys/param.h> #include <sys/conf.h> -#include <sys/fcntl.h> #include <sys/kernel.h> -#include <sys/lock.h> -#include <sys/malloc.h> +#include <sys/ioccom.h> #include <sys/module.h> -#include <sys/mutex.h> +#include <sys/priv.h> #include <sys/proc.h> -#include <sys/signalvar.h> #include <sys/systm.h> -#include <sys/uio.h> - -#include <vm/vm.h> -#include <vm/pmap.h> #include <machine/iodev.h> +#include <dev/io/iodev.h> + +static int ioopen(struct cdev *dev, int flags, int fmt, + struct thread *td); +static int ioclose(struct cdev *dev, int flags, int fmt, + struct thread *td); +static int ioioctl(struct cdev *dev, u_long cmd, caddr_t data, + int fflag, struct thread *td); + +static int iopio_read(struct iodev_pio_req *req); +static int iopio_write(struct iodev_pio_req *req); + static struct cdev *iodev; static struct cdevsw io_cdevsw = { @@ -58,6 +63,129 @@ static struct cdevsw io_cdevsw = { /* ARGSUSED */ static int +ioopen(struct cdev *dev __unused, int flags __unused, int fmt __unused, + struct thread *td) +{ + int error; + + error = priv_check(td, PRIV_IO); + if (error != 0) + return (error); + error = securelevel_gt(td->td_ucred, 0); + if (error != 0) + return (error); + error = iodev_open(td); + + return (error); +} + +/* ARGSUSED */ +static int +ioclose(struct cdev *dev __unused, int flags __unused, int fmt __unused, + struct thread *td) +{ + + return (iodev_close(td)); +} + +/* ARGSUSED */ +static int +ioioctl(struct cdev *dev __unused, u_long cmd, caddr_t data, + int fflag __unused, struct thread *td __unused) +{ + struct iodev_pio_req *pio_req; + int error; + + switch (cmd) { + case IODEV_PIO: + pio_req = (struct iodev_pio_req *)data; + switch (pio_req->access) { + case IODEV_PIO_READ: + error = iopio_read(pio_req); + break; + case IODEV_PIO_WRITE: + error = iopio_write(pio_req); + break; + default: + error = EINVAL; + break; + } + break; + default: + error = iodev_ioctl(cmd, data); + } + + return (error); +} + +static int +iopio_read(struct iodev_pio_req *req) +{ + + switch (req->width) { + case 1: + req->val = iodev_read_1(req->port); + break; + case 2: + if (req->port & 1) { + req->val = iodev_read_1(req->port); + req->val |= iodev_read_1(req->port + 1) << 8; + } else + req->val = iodev_read_2(req->port); + break; + case 4: + if (req->port & 1) { + req->val = iodev_read_1(req->port); + req->val |= iodev_read_2(req->port + 1) << 8; + req->val |= iodev_read_1(req->port + 3) << 24; + } else if (req->port & 2) { + req->val = iodev_read_2(req->port); + req->val |= iodev_read_2(req->port + 2) << 16; + } else + req->val = iodev_read_4(req->port); + break; + default: + return (EINVAL); + } + + return (0); +} + +static int +iopio_write(struct iodev_pio_req *req) +{ + + switch (req->width) { + case 1: + iodev_write_1(req->port, req->val); + break; + case 2: + if (req->port & 1) { + iodev_write_1(req->port, req->val); + iodev_write_1(req->port + 1, req->val >> 8); + } else + iodev_write_2(req->port, req->val); + break; + case 4: + if (req->port & 1) { + iodev_write_1(req->port, req->val); + iodev_write_2(req->port + 1, req->val >> 8); + iodev_write_1(req->port + 3, req->val >> 24); + } else if (req->port & 2) { + iodev_write_2(req->port, req->val); + iodev_write_2(req->port + 2, req->val >> 16); + } else + iodev_write_4(req->port, req->val); + break; + default: + return (EINVAL); + } + + return (0); +} + +/* ARGSUSED */ +static int io_modevent(module_t mod __unused, int type, void *data __unused) { switch(type) { Added: head/sys/dev/io/iodev.h ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/sys/dev/io/iodev.h Wed Apr 28 15:38:01 2010 (r207329) @@ -0,0 +1,44 @@ +/*- + * Copyright (c) 2010 Marcel Moolenaar + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + * + * $FreeBSD$ + */ + +#ifndef _DEV_IODEV_H_ +#define _DEV_IODEV_H_ + +#define IODEV_PIO_READ 0 +#define IODEV_PIO_WRITE 1 + +struct iodev_pio_req { + u_int access; + u_int port; + u_int width; + u_int val; +}; + +#define IODEV_PIO _IOWR('I', 0, struct iodev_pio_req) + +#endif /* _DEV_IODEV_H_ */ Modified: head/sys/i386/i386/io.c ============================================================================== --- head/sys/i386/i386/io.c Wed Apr 28 15:15:06 2010 (r207328) +++ head/sys/i386/i386/io.c Wed Apr 28 15:38:01 2010 (r207329) @@ -28,60 +28,32 @@ __FBSDID("$FreeBSD$"); #include <sys/param.h> -#include <sys/conf.h> -#include <sys/fcntl.h> -#include <sys/lock.h> -#include <sys/malloc.h> -#include <sys/mutex.h> -#include <sys/priv.h> #include <sys/proc.h> -#include <sys/signalvar.h> -#include <sys/systm.h> -#include <machine/db_machdep.h> #include <machine/frame.h> -#include <machine/psl.h> -#include <machine/specialreg.h> - -#include <vm/vm.h> -#include <vm/pmap.h> - #include <machine/iodev.h> +#include <machine/psl.h> -/* ARGSUSED */ int -ioopen(struct cdev *dev __unused, int flags __unused, int fmt __unused, - struct thread *td) +iodev_open(struct thread *td) { - int error; - - error = priv_check(td, PRIV_IO); - if (error != 0) - return (error); - error = securelevel_gt(td->td_ucred, 0); - if (error != 0) - return (error); td->td_frame->tf_eflags |= PSL_IOPL; - return (0); } -/* ARGSUSED */ int -ioclose(struct cdev *dev __unused, int flags __unused, int fmt __unused, - struct thread *td) +iodev_close(struct thread *td) { - td->td_frame->tf_eflags &= ~PSL_IOPL; + td->td_frame->tf_eflags &= ~PSL_IOPL; return (0); } /* ARGSUSED */ int -ioioctl(struct cdev *dev __unused, u_long cmd __unused, caddr_t data __unused, - int fflag __unused, struct thread *td __unused) +iodev_ioctl(u_long cmd __unused, caddr_t data __unused) { - return (ENXIO); + return (ENOIOCTL); } Modified: head/sys/i386/include/iodev.h ============================================================================== --- head/sys/i386/include/iodev.h Wed Apr 28 15:15:06 2010 (r207328) +++ head/sys/i386/include/iodev.h Wed Apr 28 15:38:01 2010 (r207329) @@ -25,7 +25,22 @@ * * $FreeBSD$ */ +#ifndef _MACHINE_IODEV_H_ +#define _MACHINE_IODEV_H_ -d_open_t ioopen; -d_close_t ioclose; -d_ioctl_t ioioctl; +#ifdef _KERNEL +#include <machine/cpufunc.h> + +#define iodev_read_1 inb +#define iodev_read_2 inw +#define iodev_read_4 inl +#define iodev_write_1 outb +#define iodev_write_2 outw +#define iodev_write_4 outl + +int iodev_open(struct thread *td); +int iodev_close(struct thread *td); +int iodev_ioctl(u_long cmd, caddr_t data); + +#endif /* _KERNEL */ +#endif /* _MACHINE_IODEV_H_ */ Modified: head/sys/ia64/ia64/iodev_machdep.c ============================================================================== --- head/sys/ia64/ia64/iodev_machdep.c Wed Apr 28 15:15:06 2010 (r207328) +++ head/sys/ia64/ia64/iodev_machdep.c Wed Apr 28 15:38:01 2010 (r207329) @@ -40,61 +40,33 @@ __FBSDID("$FreeBSD$"); #include <machine/efi.h> #include <machine/iodev.h> -static int iodev_pio_read(struct iodev_pio_req *req); -static int iodev_pio_write(struct iodev_pio_req *req); - static int iodev_efivar_getvar(struct iodev_efivar_req *req); static int iodev_efivar_nextname(struct iodev_efivar_req *req); static int iodev_efivar_setvar(struct iodev_efivar_req *req); /* ARGSUSED */ int -ioopen(struct cdev *dev __unused, int flags __unused, int fmt __unused, - struct thread *td) +iodev_open(struct thread *td __unused) { - int error; - - error = priv_check(td, PRIV_IO); - if (error == 0) - error = securelevel_gt(td->td_ucred, 0); - return (error); + return (0); } /* ARGSUSED */ int -ioclose(struct cdev *dev __unused, int flags __unused, int fmt __unused, - struct thread *td __unused) +iodev_close(struct thread *td __unused) { return (0); } -/* ARGSUSED */ int -ioioctl(struct cdev *dev __unused, u_long cmd, caddr_t data, - int fflag __unused, struct thread *td __unused) +iodev_ioctl(u_long cmd, caddr_t data) { struct iodev_efivar_req *efivar_req; - struct iodev_pio_req *pio_req; int error; - error = ENOIOCTL; switch (cmd) { - case IODEV_PIO: - pio_req = (struct iodev_pio_req *)data; - switch (pio_req->access) { - case IODEV_PIO_READ: - error = iodev_pio_read(pio_req); - break; - case IODEV_PIO_WRITE: - error = iodev_pio_write(pio_req); - break; - default: - error = EINVAL; - break; - } - break; case IODEV_EFIVAR: efivar_req = (struct iodev_efivar_req *)data; efivar_req->result = 0; /* So it's well-defined */ @@ -113,75 +85,11 @@ ioioctl(struct cdev *dev __unused, u_lon break; } break; - } - - return (error); -} - -static int -iodev_pio_read(struct iodev_pio_req *req) -{ - - switch (req->width) { - case 1: - req->val = bus_space_read_io_1(req->port); - break; - case 2: - if (req->port & 1) { - req->val = bus_space_read_io_1(req->port); - req->val |= bus_space_read_io_1(req->port + 1) << 8; - } else - req->val = bus_space_read_io_2(req->port); - break; - case 4: - if (req->port & 1) { - req->val = bus_space_read_io_1(req->port); - req->val |= bus_space_read_io_2(req->port + 1) << 8; - req->val |= bus_space_read_io_1(req->port + 3) << 24; - } else if (req->port & 2) { - req->val = bus_space_read_io_2(req->port); - req->val |= bus_space_read_io_2(req->port + 2) << 16; - } else - req->val = bus_space_read_io_4(req->port); - break; - default: - return (EINVAL); - } - - return (0); -} - -static int -iodev_pio_write(struct iodev_pio_req *req) -{ - - switch (req->width) { - case 1: - bus_space_write_io_1(req->port, req->val); - break; - case 2: - if (req->port & 1) { - bus_space_write_io_1(req->port, req->val); - bus_space_write_io_1(req->port + 1, req->val >> 8); - } else - bus_space_write_io_2(req->port, req->val); - break; - case 4: - if (req->port & 1) { - bus_space_write_io_1(req->port, req->val); - bus_space_write_io_2(req->port + 1, req->val >> 8); - bus_space_write_io_1(req->port + 3, req->val >> 24); - } else if (req->port & 2) { - bus_space_write_io_2(req->port, req->val); - bus_space_write_io_2(req->port + 2, req->val >> 16); - } else - bus_space_write_io_4(req->port, req->val); - break; default: - return (EINVAL); + error = ENOIOCTL; } - return (0); + return (error); } static int Modified: head/sys/ia64/include/iodev.h ============================================================================== --- head/sys/ia64/include/iodev.h Wed Apr 28 15:15:06 2010 (r207328) +++ head/sys/ia64/include/iodev.h Wed Apr 28 15:38:01 2010 (r207329) @@ -31,22 +31,16 @@ #include <sys/uuid.h> -struct iodev_pio_req { - u_int access; -#define IODEV_PIO_READ 0 -#define IODEV_PIO_WRITE 1 - u_int port; - u_int width; - u_int val; -}; - -#define IODEV_PIO _IOWR('I', 0, struct iodev_pio_req) +#ifdef _KERNEL +#include <machine/bus.h> +#endif -struct iodev_efivar_req { - u_int access; #define IODEV_EFIVAR_GETVAR 0 #define IODEV_EFIVAR_NEXTNAME 1 #define IODEV_EFIVAR_SETVAR 2 + +struct iodev_efivar_req { + u_int access; u_int result; /* errno value */ size_t namesize; u_short *name; /* UCS-2 */ @@ -59,11 +53,16 @@ struct iodev_efivar_req { #define IODEV_EFIVAR _IOWR('I', 1, struct iodev_efivar_req) #ifdef _KERNEL +#define iodev_read_1 bus_space_read_io_1 +#define iodev_read_2 bus_space_read_io_2 +#define iodev_read_4 bus_space_read_io_4 +#define iodev_write_1 bus_space_write_io_1 +#define iodev_write_2 bus_space_write_io_2 +#define iodev_write_4 bus_space_write_io_4 + +int iodev_open(struct thread *td); +int iodev_close(struct thread *td); +int iodev_ioctl(u_long, caddr_t data); -d_open_t ioopen; -d_close_t ioclose; -d_ioctl_t ioioctl; - -#endif - +#endif /* _KERNEL */ #endif /* _MACHINE_IODEV_H_ */ _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" State Changed From-To: open->patched committed in head Responsible Changed From-To: freebsd-threads->attilio same as above State Changed From-To: patched->closed MFCed/fixed by now or it will never be MFCed |
Some background: Opening /dev/io is supposed to give permission to use instructions like inb/outb. If a process doesn't have this permission, it will fault on execution of those instructions. The permission is controlled through the IOPL flag in the EEFLAGS register. On opening /dev/io, it sets the permission like so: 64 td->td_frame->tf_eflags |= PSL_IOPL; (from sys/i386/i386/io.c) Now, when a process opens a file, its fd is supposed to be shared across every thread in the process. Consequently, one would think that the permissions associated with this fd would also be shared. However, this is not the case. It seems that only threads who are descendents of a thread that had /dev/io opened when they were created. Threads created by threads without the permissioin will not get the permission. Threads which exist at the time /dev/io is opened, other than the opening thread, will also not get the permission. So, a process will need to make sure that every thread needing IO permissions has an ancestor that opened /dev/io before the thread was created. This seems sub-optimal, and contrary to the spirit of sharing file descriptors over an entire process. To make the problem worse, closing /dev/io works in the same manner: 74 td->td_frame->tf_eflags &= ~PSL_IOPL; This leads me to believe that every thread which inherited permissions from an ancestor thread will keep the permissions, despite the closure of /dev/io. In fact, since the fd is shared across the process, a thread without the permissions could close the file. I think that, for the same reason that the flag isn't propagated to all threads on opening /dev/io, it will in turn not be unset in all threads when closing /dev/io. So, we could have a process which doesn't have /dev/io open still have IO permissions. This would be a security hole. I also looked at how the linux compat code handles iopl for i386, and it is similar to this (it sets some flags in td->frame), so the issue might also crop up there. How-To-Repeat: I haven't tested the removal of permissions portion, but I have tested that permissions aren't shared across the processes. I wrote a program that starts a thread, opens /dev/io in the main thread, then tries to write to the serial port in the second thread. It gives a bus error. include <pthread.h> #include <string.h> #include <stdlib.h> #include <stdio.h> #include <fcntl.h> #include <machine/cpufunc.h> pthread_cond_t dev_cond; pthread_mutex_t dev_lock; pthread_t thr; void io_main(); void do_write(); void *second_thread(void*); int main(int argc, char ** argv) { if(pthread_cond_init(&dev_cond, NULL)) { printf("Error initializing condition var.\n"); return 1; } if(pthread_mutex_init(&dev_lock, NULL)) { printf("Error initializing mutex.\n"); return 1; } io_main(); return 0; } void do_write() { outb(0x378, 'a'); printf("done\n"); } void io_main() { int fd; pthread_mutex_lock(&dev_lock); if(pthread_create(&thr,NULL, second_thread, NULL)) { printf("Error creating second thread\n"); exit(1); } if((fd = open("/dev/io", O_RDWR)) == -1) { printf("Error opening /dev/io: %i\n", fd); exit(1); } pthread_cond_wait(&dev_cond,&dev_lock); close(fd); } void *second_thread(void *args) { int fd; pthread_mutex_lock(&dev_lock); printf("Performing write in secondary...\n"); do_write(); pthread_mutex_unlock(&dev_lock); pthread_cond_signal(&dev_cond); return 0; }