Page MenuHomeFreeBSD

Use a consistent snapshot of the fd's rights in fget_mmap().
ClosedPublic

Authored by markj on Jun 28 2019, 10:13 PM.

Details

Summary

fget_mmap() extracts the max_prot flags corresponding to rights on the
descriptor. The issue is, those rights may change if another thread
closes the fd or something like that. This is caught using the sequence
counter, but not before it can trip assertions in the cap_rights code.

Instead, make a local copy of the rights, using seqc to ensure that the
copy is consistent, and use that to derive max_prot. This is what
fget_unlocked() does as well.

I quickly looked for other places where we might have a similar
problem and didn't find any, but will do a more careful audit before
committing. We hold the fildesc lock in most places that extract the
capability rights from an fd.

Test Plan

syzkaller found the issue and helpfully generated a test case that
reproduces the race (given enough trials). I've been running it with
this patch applied.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Jun 28 2019, 10:13 PM
markj edited the test plan for this revision. (Show Details)Jun 28 2019, 10:15 PM
markj added reviewers: mjg, brooks, oshogbo.
brooks accepted this revision.Jun 28 2019, 10:23 PM

LGTM

This revision is now accepted and ready to land.Jun 28 2019, 10:23 PM
mjg added a comment.Jun 28 2019, 11:17 PM

I don't think it's useful to move it out, it's an avoidable branch. i.e. I think it would be better to:

if (maxprotp != NULL) {
    fdrights = *cap_rights(fdp, fd);
    *maxprotp = cap_rights_to_vmprot(&fdrights);
}

In normal operation the loop is never supposed to restart.

Side note is that fget_unlocked indeed takes a stable snapshot before evaluating caps, but I was considering not doing it in order to save some work in the common case. As it is we can safely operate even on changing creds and then validate the result by checking seqc. So perhaps would be nicer to just take care of whatever assertions which end up being tripped over.

mjg accepted this revision.Jun 29 2019, 4:13 AM

Half-scratch my previous comment. Of course at the time the state is already possibly in flux so it has to be moved away if assertions are to be kept. See the other point though.

markj added a comment.Jun 29 2019, 4:00 PM
In D20800#450259, @mjg wrote:

Side note is that fget_unlocked indeed takes a stable snapshot before evaluating caps, but I was considering not doing it in order to save some work in the common case. As it is we can safely operate even on changing creds and then validate the result by checking seqc. So perhaps would be nicer to just take care of whatever assertions which end up being tripped over.

To be honest, I am a little skeptical that we should introduce data races in the capability code just to avoid a 16-byte copy to the stack. That would make it quite difficult to reason about the correctness of the code.

This revision was automatically updated to reflect the committed changes.
mjg added a comment.Jun 29 2019, 7:19 PM

It's not about just copying. fget_unlocked is severely pessimized right now and so happens to amount of __predict_true/false convinces clang to not generated a jump fest for the common case.

No matter what the result of cap_* funcs, it is only checked after seqc verified you were operating on a stable snapshot from the get go. Thus incorrectly denying or allowing due to transient state is of no significance. The only factor is to ensure the data operated on is not freed and that currently is a given.