Re: svn commit: r272505 - in head/sys: kern sys
On Sat, Oct 04, 2014 at 02:21:54PM +, Bjoern A. Zeeb wrote: On 04 Oct 2014, at 08:08 , Mateusz Guzik m...@freebsd.org wrote: Author: mjg Date: Sat Oct 4 08:08:56 2014 New Revision: 272505 URL: https://svnweb.freebsd.org/changeset/base/272505 Log: Plug capability races. fp and appropriate capability lookups were not atomic, which could result in improper capabilities being checked. This could result either in protection bypass or in a spurious ENOTCAPABLE. Make fp + capability check atomic with the help of sequence counters. Reviewed by: kib MFC after: 3 weeks Modified: head/sys/kern/kern_descrip.c head/sys/sys/filedesc.h … This file is included from user space. There is no opt_capsicum.h there. Including an opt_* in the header file seems wrong in a lot of ways usually. I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse. This needs a better fix. Hi, this also breaks the nvidia-driver port (also with your fix). BR, Stefan ___ 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-unsubscr...@freebsd.org
Re: svn commit: r272505 - in head/sys: kern sys
On Sun, Oct 05, 2014 at 06:39:54PM +0200, Stefan Farfeleder wrote: On Sat, Oct 04, 2014 at 02:21:54PM +, Bjoern A. Zeeb wrote: On 04 Oct 2014, at 08:08 , Mateusz Guzik m...@freebsd.org wrote: Author: mjg Date: Sat Oct 4 08:08:56 2014 New Revision: 272505 URL: https://svnweb.freebsd.org/changeset/base/272505 Log: Plug capability races. fp and appropriate capability lookups were not atomic, which could result in improper capabilities being checked. This could result either in protection bypass or in a spurious ENOTCAPABLE. Make fp + capability check atomic with the help of sequence counters. Reviewed by: kib MFC after: 3 weeks Modified: head/sys/kern/kern_descrip.c head/sys/sys/filedesc.h ??? This file is included from user space. There is no opt_capsicum.h there. Including an opt_* in the header file seems wrong in a lot of ways usually. I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse. This needs a better fix. Hi, this also breaks the nvidia-driver port (also with your fix). Is the breakage due to missing opt_capsicum.h file ? If yes, what I proposed, i.e. making the new member unconditional, should fix it without changes to the module build system. ___ 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-unsubscr...@freebsd.org
Re: svn commit: r272505 - in head/sys: kern sys
On Sun, Oct 05, 2014 at 08:16:17PM +0300, Konstantin Belousov wrote: On Sun, Oct 05, 2014 at 06:39:54PM +0200, Stefan Farfeleder wrote: On Sat, Oct 04, 2014 at 02:21:54PM +, Bjoern A. Zeeb wrote: On 04 Oct 2014, at 08:08 , Mateusz Guzik m...@freebsd.org wrote: Author: mjg Date: Sat Oct 4 08:08:56 2014 New Revision: 272505 URL: https://svnweb.freebsd.org/changeset/base/272505 Log: Plug capability races. fp and appropriate capability lookups were not atomic, which could result in improper capabilities being checked. This could result either in protection bypass or in a spurious ENOTCAPABLE. Make fp + capability check atomic with the help of sequence counters. Reviewed by: kib MFC after: 3 weeks Modified: head/sys/kern/kern_descrip.c head/sys/sys/filedesc.h ??? This file is included from user space. There is no opt_capsicum.h there. Including an opt_* in the header file seems wrong in a lot of ways usually. I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse. This needs a better fix. Hi, this also breaks the nvidia-driver port (also with your fix). Is the breakage due to missing opt_capsicum.h file ? If yes, what I proposed, i.e. making the new member unconditional, should fix it without changes to the module build system. Yes, it breaks due to the missing file. Stefan ___ 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-unsubscr...@freebsd.org
Re: svn commit: r272505 - in head/sys: kern sys
On Sun, Oct 05, 2014 at 08:19:06PM +0200, Stefan Farfeleder wrote: On Sun, Oct 05, 2014 at 08:16:17PM +0300, Konstantin Belousov wrote: On Sun, Oct 05, 2014 at 06:39:54PM +0200, Stefan Farfeleder wrote: On Sat, Oct 04, 2014 at 02:21:54PM +, Bjoern A. Zeeb wrote: On 04 Oct 2014, at 08:08 , Mateusz Guzik m...@freebsd.org wrote: Author: mjg Date: Sat Oct 4 08:08:56 2014 New Revision: 272505 URL: https://svnweb.freebsd.org/changeset/base/272505 Log: Plug capability races. fp and appropriate capability lookups were not atomic, which could result in improper capabilities being checked. This could result either in protection bypass or in a spurious ENOTCAPABLE. Make fp + capability check atomic with the help of sequence counters. Reviewed by: kib MFC after: 3 weeks Modified: head/sys/kern/kern_descrip.c head/sys/sys/filedesc.h ??? This file is included from user space. There is no opt_capsicum.h there. Including an opt_* in the header file seems wrong in a lot of ways usually. I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse. This needs a better fix. Hi, this also breaks the nvidia-driver port (also with your fix). Is the breakage due to missing opt_capsicum.h file ? If yes, what I proposed, i.e. making the new member unconditional, should fix it without changes to the module build system. Yes, it breaks due to the missing file. Can you update the kernel to at least r272569 and test again? Make sure you are running the new kernel before testing the driver. -- Mateusz Guzik mjguzik gmail.com ___ 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-unsubscr...@freebsd.org
svn commit: r272505 - in head/sys: kern sys
Author: mjg Date: Sat Oct 4 08:08:56 2014 New Revision: 272505 URL: https://svnweb.freebsd.org/changeset/base/272505 Log: Plug capability races. fp and appropriate capability lookups were not atomic, which could result in improper capabilities being checked. This could result either in protection bypass or in a spurious ENOTCAPABLE. Make fp + capability check atomic with the help of sequence counters. Reviewed by: kib MFC after:3 weeks Modified: head/sys/kern/kern_descrip.c head/sys/sys/filedesc.h Modified: head/sys/kern/kern_descrip.c == --- head/sys/kern/kern_descrip.cSat Oct 4 08:05:39 2014 (r272504) +++ head/sys/kern/kern_descrip.cSat Oct 4 08:08:56 2014 (r272505) @@ -288,11 +288,18 @@ _fdfree(struct filedesc *fdp, int fd, in struct filedescent *fde; fde = fdp-fd_ofiles[fd]; +#ifdef CAPABILITIES + if (!last) + seq_write_begin(fde-fde_seq); +#endif filecaps_free(fde-fde_caps); if (last) return; - bzero(fde, sizeof(*fde)); + bzero(fde_change(fde), fde_change_size); fdunused(fdp, fd); +#ifdef CAPABILITIES + seq_write_end(fde-fde_seq); +#endif } static inline void @@ -883,13 +890,19 @@ do_dup(struct thread *td, int flags, int /* * Duplicate the source descriptor. */ +#ifdef CAPABILITIES + seq_write_begin(newfde-fde_seq); +#endif filecaps_free(newfde-fde_caps); - *newfde = *oldfde; + memcpy(fde_change(newfde), fde_change(oldfde), fde_change_size); filecaps_copy(oldfde-fde_caps, newfde-fde_caps); if ((flags DUP_CLOEXEC) != 0) newfde-fde_flags = oldfde-fde_flags | UF_EXCLOSE; else newfde-fde_flags = oldfde-fde_flags ~UF_EXCLOSE; +#ifdef CAPABILITIES + seq_write_end(newfde-fde_seq); +#endif *retval = new; if (delfp != NULL) { @@ -1756,6 +1769,9 @@ finstall(struct thread *td, struct file } fhold(fp); fde = fdp-fd_ofiles[*fd]; +#ifdef CAPABILITIES + seq_write_begin(fde-fde_seq); +#endif fde-fde_file = fp; if ((flags O_CLOEXEC) != 0) fde-fde_flags |= UF_EXCLOSE; @@ -1763,6 +1779,9 @@ finstall(struct thread *td, struct file filecaps_move(fcaps, fde-fde_caps); else filecaps_fill(fde-fde_caps); +#ifdef CAPABILITIES + seq_write_end(fde-fde_seq); +#endif FILEDESC_XUNLOCK(fdp); return (0); } @@ -2294,6 +2313,7 @@ fget_unlocked(struct filedesc *fdp, int struct file *fp; u_int count; #ifdef CAPABILITIES + seq_t seq; cap_rights_t haverights; int error; #endif @@ -2314,7 +2334,12 @@ fget_unlocked(struct filedesc *fdp, int */ for (;;) { #ifdef CAPABILITIES + seq = seq_read(fd_seq(fdp, fd)); fde = fdp-fd_ofiles[fd]; + if (!seq_consistent(fd_seq(fdp, fd), seq)) { + cpu_spinwait(); + continue; + } fp = fde.fde_file; #else fp = fdp-fd_ofiles[fd].fde_file; @@ -2343,7 +2368,11 @@ fget_unlocked(struct filedesc *fdp, int */ if (atomic_cmpset_acq_int(fp-f_count, count, count + 1) != 1) continue; +#ifdef CAPABILITIES + if (seq_consistent_nomb(fd_seq(fdp, fd), seq)) +#else if (fp == fdp-fd_ofiles[fd].fde_file) +#endif break; fdrop(fp, curthread); } @@ -2700,6 +2729,7 @@ int dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode, int openerror, int *indxp) { + struct filedescent *newfde, *oldfde; struct file *fp; int error, indx; @@ -2743,17 +2773,32 @@ dupfdopen(struct thread *td, struct file return (EACCES); } fhold(fp); - fdp-fd_ofiles[indx] = fdp-fd_ofiles[dfd]; - filecaps_copy(fdp-fd_ofiles[dfd].fde_caps, - fdp-fd_ofiles[indx].fde_caps); + newfde = fdp-fd_ofiles[indx]; + oldfde = fdp-fd_ofiles[dfd]; +#ifdef CAPABILITIES + seq_write_begin(newfde-fde_seq); +#endif + memcpy(fde_change(newfde), fde_change(oldfde), fde_change_size); + filecaps_copy(oldfde-fde_caps, newfde-fde_caps); +#ifdef CAPABILITIES + seq_write_end(newfde-fde_seq); +#endif break; case ENXIO: /* * Steal away the file pointer from dfd and stuff it into indx. */ - fdp-fd_ofiles[indx] = fdp-fd_ofiles[dfd]; - bzero(fdp-fd_ofiles[dfd], sizeof(fdp-fd_ofiles[dfd])); + newfde = fdp-fd_ofiles[indx]; +
Re: svn commit: r272505 - in head/sys: kern sys
On 04 Oct 2014, at 08:08 , Mateusz Guzik m...@freebsd.org wrote: Author: mjg Date: Sat Oct 4 08:08:56 2014 New Revision: 272505 URL: https://svnweb.freebsd.org/changeset/base/272505 Log: Plug capability races. fp and appropriate capability lookups were not atomic, which could result in improper capabilities being checked. This could result either in protection bypass or in a spurious ENOTCAPABLE. Make fp + capability check atomic with the help of sequence counters. Reviewed by: kib MFC after: 3 weeks Modified: head/sys/kern/kern_descrip.c head/sys/sys/filedesc.h … This file is included from user space. There is no opt_capsicum.h there. Including an opt_* in the header file seems wrong in a lot of ways usually. I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse. This needs a better fix. I also wonder why the (conditional) fde_seq ended up at the beginning of the structure rather than the end? Modified: head/sys/sys/filedesc.h == --- head/sys/sys/filedesc.h Sat Oct 4 08:05:39 2014(r272504) +++ head/sys/sys/filedesc.h Sat Oct 4 08:08:56 2014(r272505) @@ -33,11 +33,14 @@ #ifndef _SYS_FILEDESC_H_ #define _SYS_FILEDESC_H_ +#include opt_capsicum.h + #include sys/caprights.h #include sys/queue.h #include sys/event.h #include sys/lock.h #include sys/priority.h +#include sys/seq.h #include sys/sx.h #include machine/_limits.h @@ -50,6 +53,9 @@ struct filecaps { }; struct filedescent { +#ifdef CAPABILITIES + seq_tfde_seq; /* if you need fde_file and fde_caps in sync */ +#endif struct file *fde_file; /* file structure for open file */ struct filecaps fde_caps; /* per-descriptor rights */ uint8_t fde_flags; /* per-process open file flags */ @@ -58,6 +64,13 @@ struct filedescent { #define fde_fcntls fde_caps.fc_fcntls #define fde_ioctls fde_caps.fc_ioctls #define fde_nioctls fde_caps.fc_nioctls +#ifdef CAPABILITIES +#define fde_change(fde) ((char *)(fde) + sizeof(seq_t)) +#define fde_change_size (sizeof(struct filedescent) - sizeof(seq_t)) +#else +#define fde_change(fde) ((fde)) +#define fde_change_size (sizeof(struct filedescent)) +#endif /* * This structure is used for the management of descriptors. It may be @@ -82,6 +95,9 @@ struct filedesc { int fd_holdleaderscount;/* block fdfree() for shared close() */ int fd_holdleaderswakeup; /* fdfree() needs wakeup */ }; +#ifdef CAPABILITIES +#define fd_seq(fdp, fd) ((fdp)-fd_ofiles[(fd)].fde_seq) +#endif /* * Structure to keep track of (process leader, struct fildedesc) tuples. — Bjoern A. Zeeb Come on. Learn, goddamn it., WarGames, 1983 ___ 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-unsubscr...@freebsd.org
Re: svn commit: r272505 - in head/sys: kern sys
On Sat, Oct 04, 2014 at 02:21:54PM +, Bjoern A. Zeeb wrote: On 04 Oct 2014, at 08:08 , Mateusz Guzik m...@freebsd.org wrote: Author: mjg Date: Sat Oct 4 08:08:56 2014 New Revision: 272505 URL: https://svnweb.freebsd.org/changeset/base/272505 Log: Plug capability races. fp and appropriate capability lookups were not atomic, which could result in improper capabilities being checked. This could result either in protection bypass or in a spurious ENOTCAPABLE. Make fp + capability check atomic with the help of sequence counters. Reviewed by: kib MFC after: 3 weeks Modified: head/sys/kern/kern_descrip.c head/sys/sys/filedesc.h ? This file is included from user space. There is no opt_capsicum.h there. Including an opt_* in the header file seems wrong in a lot of ways usually. I think that easiest, and probably the most correct, fix is to include the fde_seq member unconditionally. I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse. This needs a better fix. Hm, I do see inclusion of sys/filedesc.h in the usermode programs, most worrying is libprocstat. But, there is nothing useful for usermode in the header, except possibly for the code with inspects KVA. I also wonder why the (conditional) fde_seq ended up at the beginning of the structure rather than the end? Why not ? ___ 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-unsubscr...@freebsd.org
Re: svn commit: r272505 - in head/sys: kern sys
On 04 Oct 2014, at 16:36 , Konstantin Belousov kostik...@gmail.com wrote: On Sat, Oct 04, 2014 at 02:21:54PM +, Bjoern A. Zeeb wrote: On 04 Oct 2014, at 08:08 , Mateusz Guzik m...@freebsd.org wrote: Author: mjg Date: Sat Oct 4 08:08:56 2014 New Revision: 272505 URL: https://svnweb.freebsd.org/changeset/base/272505 Log: Plug capability races. fp and appropriate capability lookups were not atomic, which could result in improper capabilities being checked. This could result either in protection bypass or in a spurious ENOTCAPABLE. Make fp + capability check atomic with the help of sequence counters. Reviewed by:kib MFC after: 3 weeks Modified: head/sys/kern/kern_descrip.c head/sys/sys/filedesc.h ? This file is included from user space. There is no opt_capsicum.h there. Including an opt_* in the header file seems wrong in a lot of ways usually. I think that easiest, and probably the most correct, fix is to include the fde_seq member unconditionally. I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse. This needs a better fix. Hm, I do see inclusion of sys/filedesc.h in the usermode programs, most worrying is libprocstat. But, there is nothing useful for usermode in the header, except possibly for the code with inspects KVA. It’s included indirectly imho through other sys/* header files if I am not mistaken. I also wonder why the (conditional) fde_seq ended up at the beginning of the structure rather than the end? Why not ? Because it guarantees the structure layout (offsets) to change for either way, where-as at the end things would at least be deterministic for the beginning; it might not make a change in reality, but it’s nice anyway (also for debugging). — Bjoern A. Zeeb Come on. Learn, goddamn it., WarGames, 1983 ___ 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-unsubscr...@freebsd.org
Re: svn commit: r272505 - in head/sys: kern sys
On Sat, 4 Oct 2014, Bjoern A. Zeeb wrote: On 04 Oct 2014, at 08:08 , Mateusz Guzik m...@freebsd.org wrote: ... Log: Plug capability races. ... This file is included from user space. There is no opt_capsicum.h there. Including an opt_* in the header file seems wrong in a lot of ways usually. I tried to add a bandaid for the moment with r272523 which (to be honest) makes it worse. This needs a better fix. I also wonder why the (conditional) fde_seq ended up at the beginning of the structure rather than the end? It adds to the already-massive namespace pollution in other ways. Modified: head/sys/sys/filedesc.h == --- head/sys/sys/filedesc.h Sat Oct 4 08:05:39 2014(r272504) +++ head/sys/sys/filedesc.h Sat Oct 4 08:08:56 2014(r272505) @@ -33,11 +33,14 @@ #ifndef _SYS_FILEDESC_H_ #define _SYS_FILEDESC_H_ +#include opt_capsicum.h + Fatal namspace pollution. #include sys/caprights.h #include sys/queue.h #include sys/event.h #include sys/lock.h #include sys/priority.h Old massive namespace pollution. +#include sys/seq.h New namespace pollution. #include sys/sx.h #include machine/_limits.h Old massive namespace pollution, continued. The pollution is nested. sys/sx.h at least has a _KERNEL ifdef around most of its pollution. @@ -50,6 +53,9 @@ struct filecaps { }; struct filedescent { +#ifdef CAPABILITIES + seq_tfde_seq; /* if you need fde_file and fde_caps in sync */ +#endif This ifdef makes the struct not really a struct. The layout depends on visible options, and the visible options may depend on pollution. The worst sort of pollution bug would occur if something else has a similar ifdef but doesn't include the options header. Then when different pollution there results in including this header, the include of the options header here gives different visible options and thus a different struct layout there. struct file *fde_file; /* file structure for open file */ struct filecaps fde_caps; /* per-descriptor rights */ uint8_t fde_flags; /* per-process open file flags */ @@ -58,6 +64,13 @@ struct filedescent { #define fde_fcntls fde_caps.fc_fcntls #define fde_ioctls fde_caps.fc_ioctls #define fde_nioctls fde_caps.fc_nioctls +#ifdef CAPABILITIES +#definefde_change(fde) ((char *)(fde) + sizeof(seq_t)) +#definefde_change_size (sizeof(struct filedescent) - sizeof(seq_t)) +#else +#definefde_change(fde) ((fde)) +#definefde_change_size (sizeof(struct filedescent)) +#endif /* * This structure is used for the management of descriptors. It may be @@ -82,6 +95,9 @@ struct filedesc { int fd_holdleaderscount;/* block fdfree() for shared close() */ int fd_holdleaderswakeup; /* fdfree() needs wakeup */ }; +#ifdef CAPABILITIES +#definefd_seq(fdp, fd) ((fdp)-fd_ofiles[(fd)].fde_seq) +#endif The Ifdefs of the macros are not even wrong. Macros are not as polluting as inline functions, so always defining them doen't cause many problems. They just give relatively minor namespace pollution and are unusable if the headers needed to use them are not included. The patch has some style bugs, e.g., long lines. Bruce ___ 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-unsubscr...@freebsd.org