Re: svn commit: r272505 - in head/sys: kern sys

2014-10-05 Thread Stefan Farfeleder
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

2014-10-05 Thread Konstantin Belousov
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

2014-10-05 Thread Stefan Farfeleder
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

2014-10-05 Thread Mateusz Guzik
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

2014-10-04 Thread Mateusz Guzik
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

2014-10-04 Thread Bjoern A. Zeeb

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

2014-10-04 Thread Konstantin Belousov
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

2014-10-04 Thread Bjoern A. Zeeb

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

2014-10-04 Thread Bruce Evans

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