Re: [syzbot] [net?] [v9fs?] KCSAN: data-race in p9_fd_create / p9_fd_create (2)

[Dominique Martinet]

Marco Elver wrote on Tue, Aug 29, 2023 at 04:11:38PM +0200:
> On Tue, Aug 29, 2023 at 07:57PM +0900, Dominique Martinet wrote:
> [...]
> > Yes well that doesn't seem too hard to hit, both threads are just
> > setting O_NONBLOCK to the same fd in parallel (0x800 is 04000,
> > O_NONBLOCK)
> > 
> > I'm not quite sure why that'd be a problem; and I'm also pretty sure
> > that wouldn't work anyway (9p has no muxing or anything that'd allow
> > sharing the same fd between multiple mounts)
> > 
> > Can this be flagged "don't care" ?
> 
> If it's an intentional data race, it could be marked data_race() [1].
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/access-marking.txt

Thanks!

> However, staring at this code for a bit, I wonder why the f_flags are
> set on open, and not on initialization somewhere...

This open is during the mount initialization (mount/p9_client_create,
full path in the stack); there's no more initialization-ish code we
have.
The problem here is that we allow to pass any old arbitrary fd, so the
user can open their fd how they want and abuse mount to use it on
multiple mounts, even if that has no way of working (as I mentionned,
there's no control flow at all -- you'll create two completely separate
client state machines that'll both try to read and/or write (separate
fds) on the same fd, and it'll all get jumbled up.
> 
> Anyway, a patch like the below would document that the data race is
> intended and we assume that there is no way (famous last words) the
> compiler or the CPU can mess it up (and KCSAN won't report it again).

That's good enough for me as my position really is just "don't do
that"... Would that also protect from syzcaller sending the fd to mount
on one side, and calling fcntl(F_SETFL) on the side?
At this rate we might as well just take the file's f_lock as setfl does,
but perhaps there's a way to steal the fd from userspace somehow?

It's not just "don't use this fd for another mount", it really is "don't
use this fd anymore while it is used by a mount".

This is made complicated that we only want to steal half of the fd, you
could imagine a weird setup like this:

 ┌────────────────────────────────────┐         ┌─────────────────┐
 │                                    │         │                 │
 │                                    │         │  kernel client  │
 │   fd3 tcp to server                │         │                 │
 │       write end  ◄─────────────────┼─────────┤                 │
 │                                    │         │                 │
 │       read end   ──┐               │         │                 │
 │                    │               │         │                 │
 │   fd4 pipeA        │ MITMing...    │         │                 │
 │                    │               │         │                 │
 │       write end  ◄─┘               │         │                 │
 │                                    │         │                 │
 │   fd5 pipeB                        │         │                 │
 │                                    │         │                 │
 │       read end  ───────────────────┼────────►│                 │
 │                                    │         │                 │
 │                                    │         │                 │
 └────────────────────────────────────┘         └─────────────────┘

I'm not sure we actually want to support something like that, but it's
currently possible and making mount act like close() on the fd would
break this... :|

So, yeah, well; this is one of these "please don't do this" that
syzcaller has no way of knowing about; it's good to test (please don't
do this has no security guarantee so the kernel shouldn't blow up!),
but if the only fallout is breakage then yeah data_race() is fine.

Compilers and/or CPU might be able to blow this out of proportion, but
hopefully they won't go around modifying another unrelated value in
memory somewhere, and we do fdget so it shouldn't turn into a UAF, so I
guess it's fine?... Just taking f_lock here won't solve anything and
might give the impression we support concurrent uses.


Sorry for rambling, and thanks for the patch; I'm not sure if Eric has
anything planned for next cycle but either of us can take it and call it
a day.
-- 
Dominique Martinet | Asmadeus