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

[markj]

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.

[brooks]

LGTM

[mjg]

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]

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]

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.

[mjg]

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.