[PATCH 0/2] plug capability races

[Mateusz Guzik]

On Fri, Aug 15, 2014 at 10:31:45AM -0400, John Baldwin wrote:
> On Thursday, August 14, 2014 8:55:10 pm Mateusz Guzik wrote:
> > fget_unlocked currently reads 'fde' which is a structure consisting of
> > serveral fields. In effect the read is inatomic and may result in
> > obtaining file pointer with stale or incorrect capabilities.
> > 
> > Example race is with dup2.
> > 
> > Side effect is that capability checks can be circumvented.
> > 
> > Proposed way to fix it is with the help of sequence counters.
> > 
> > Patchset assumes stuff from
> > 'Getting rid of atomic_load_acq_int(&fdp->fd_nfiles)) from fget_unlocked'
> > ( http://lists.freebsd.org/pipermail/freebsd-arch/2014-July/015550.html )
> > is applied. There is no technical dependency between patches (apart from
> > READ_ONCE), but this patch amortizes performance hit introduced with 
> seqlock.
> > 
> > So this introduces a measurable hit with a microbenchmark (16 threads
> > reading from a pipe which fails with EAGAIN), but is still much faster than
> > current code with atomic_load_acq_int(&fdp->fd_nfiles).
> > 
> > x propernoacq-readpipe-run-sum
> > + seq2-noacq-readpipe-run-sum
> > N           Min           Max        Median           Avg        Stddev
> > x  20      59479718      59527286      59496714      59499504     13752.968
> > +  20      54520752      54920054      54829539      54773480     136842.96
> > Difference at 95.0% confidence
> >     	-4.72602e+06 +/- 62244.4
> > 	-7.94296% +/- 0.104613%
> > 	(Student's t, pooled s = 97250)
> > 
> > There is still one theoretical race unfixed, but I don't believe it matters
> > much.
> > 
> > The race is:
> > fp gets reallocated before refcount check. this resuls in returning fp
> > regardless of new caps, but I don't see how this particular race could be
> > exploited. It could be fixed by re-reading entire fde and checking if it
> > changed.
> 
> One thing I would like to see is for the timecounter code to be adapted to use 
> the seq API instead of doing it by hand (the timecounter code is also missing 
> barriers due to doing it by hand).
> 
> Also, seq needs a seq(9) manpage.
> 

Sure, I'm happy to write one later.

Just in case I would like to note the following:

There are some similarities to linux version of the same idea:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/seqlock.h

On the other hand I don't see how one could implement this in a vastly
different matter.

-- 
Mateusz Guzik <mjguzik gmail.com>