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.c        Sat Oct  4 08:05:39 2014        
(r272504)
+++ head/sys/kern/kern_descrip.c        Sat 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];
+               oldfde = &fdp->fd_ofiles[dfd];
+#ifdef CAPABILITIES
+               seq_write_begin(&newfde->fde_seq);
+#endif
+               memcpy(fde_change(newfde), fde_change(oldfde), fde_change_size);
+               bzero(fde_change(oldfde), fde_change_size);
                fdunused(fdp, dfd);
+#ifdef CAPABILITIES
+               seq_write_end(&newfde->fde_seq);
+#endif
                break;
        }
        FILEDESC_XUNLOCK(fdp);

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_t            fde_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.
_______________________________________________
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"

Reply via email to