Re: [PATCH v2 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Christian Schoenebeck]

On Freitag, 22. April 2022 04:43:40 CEST Akihiko Odaki wrote:
> On 2022/04/22 0:07, Christian Schoenebeck wrote:
> > mknod() on macOS does not support creating sockets, so divert to
> > call sequence socket(), bind() and chmod() respectively if S_IFSOCK
> > was passed with mode argument.
> > 
> > Link: https://lore.kernel.org/qemu-devel/17933734.zYzKuhC07K@silver/
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > Reviewed-by: Will Cohen <wwcohen@gmail.com>
> > ---
> > 
> >   hw/9pfs/9p-util-darwin.c | 27 ++++++++++++++++++++++++++-
> >   1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> > index e24d09763a..39308f2a45 100644
> > --- a/hw/9pfs/9p-util-darwin.c
> > +++ b/hw/9pfs/9p-util-darwin.c
> > @@ -74,6 +74,27 @@ int fsetxattrat_nofollow(int dirfd, const char
> > *filename, const char *name,> 
> >    */
> >   
> >   #if defined CONFIG_PTHREAD_FCHDIR_NP
> > 
> > +static int create_socket_file_at_cwd(const char *filename, mode_t mode) {
> > +    int fd, err;
> > +    struct sockaddr_un addr = {
> > +        .sun_family = AF_UNIX
> > +    };
> > +
> > +    fd = socket(PF_UNIX, SOCK_DGRAM, 0);
> > +    if (fd == -1) {
> > +        return fd;
> > +    }
> > +    snprintf(addr.sun_path, sizeof(addr.sun_path), "./%s", filename);
> 
> It would result in an incorrect path if the path does not fit in
> addr.sun_path. It should report an explicit error instead.

Looking at its header file, 'sun_path' is indeed defined on macOS with an 
oddly small size of only 104 bytes. So yes, I should explicitly handle that 
error case.

I'll post a v3.

> > +    err = bind(fd, (struct sockaddr *) &addr, sizeof(addr));
> > +    if (err == -1) {
> > +        goto out;
> 
> You may close(fd) as soon as bind() returns (before checking the
> returned value) and eliminate goto.

Yeah, I thought about that alternative, but found it a bit ugly, and probably 
also counter-productive in case this function might get extended with more 
error pathes in future. Not that I would insist on the current solution 
though.

> > +    }
> > +    err = chmod(addr.sun_path, mode);
> 
> I'm not sure if it is fine to have a time window between bind() and
> chmod(). Do you have some rationale?

Good question. QEMU's 9p server is multi-threaded; all 9p requests come in 
serialized and the 9p server controller portion (9p.c) is only running on QEMU 
main thread, but the actual filesystem driver calls are then dispatched to 
QEMU worker threads and therefore running concurrently at this point:

https://wiki.qemu.org/Documentation/9p#Threads_and_Coroutines

Similar situation on Linux 9p client side: it handles access to a mounted 9p 
filesystem concurrently, requests are then serialized by 9p driver on Linux 
and sent over wire to 9p server (host).

So yes, there might be implications by that short time windows. But could that 
be exploited on macOS hosts in practice?

The socket file would have mode srwxr-xr-x for a short moment.

For security_model=mapped* this should not be a problem.

For security_model=none|passhrough, in theory, maybe? But how likely is that? 
If you are using a Linux client for instance, trying to brute-force opening 
the socket file, the client would send several 9p commands (Twalk, Tgetattr, 
Topen, probably more). The time window of the two commands above should be 
much smaller than that and I would expect one of the 9p commands to error out 
in between.

What would be a viable approach to avoid this issue on macOS?

> > +out:
> > +    close(fd);
> > +    return err;
> > +}
> > +
> > 
> >   int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t
> >   dev)
> >   {
> >   
> >       int preserved_errno, err;
> > 
> > @@ -93,7 +114,11 @@ int qemu_mknodat(int dirfd, const char *filename,
> > mode_t mode, dev_t dev)> 
> >       if (pthread_fchdir_np(dirfd) < 0) {
> >       
> >           return -1;
> >       
> >       }
> > 
> > -    err = mknod(filename, mode, dev);
> > +    if (S_ISSOCK(mode)) {
> > +        err = create_socket_file_at_cwd(filename, mode);
> > +    } else {
> > +        err = mknod(filename, mode, dev);
> > +    }
> > 
> >       preserved_errno = errno;
> >       /* Stop using the thread-local cwd */
> >       pthread_fchdir_np(-1);