Re: [PATCH] linux-user/syscall: zero-init msghdr in do_sendrecvmsg_locked

[Kenta Iwasaki <kenta@lithdew.net>]

[-- Attachment #1: Type: text/plain, Size: 4266 bytes --]

Sure,

The bytes of `msghdr` need to be cleared because the `msghdr` struct layout
specified in QEMU appears to generalize between the definitions of `msghdr`
across different libc's and kernels. To appropriately generalize `msghdr`
across libc's and kernels would either:

1. require specializing code in do_sendrecvmsg_locked() for each individual
libc and kernel version, or
2. zeroing out all bytes of `msghdr`, b/c certain libc or kernel versions
may misinterpret the undefined padding bytes that come from misalignment in
the struct as actual syscall params.

The patch I provided would be going for route #2, given that it's a simpler
fix for the underlying problem for the short term.

What I believe is the background behind why the struct layout has been a
problem is because, since the beginning, the Linux kernel has always
specified the layout of `msghdr` differently from POSIX. Given that this
implies incompatibility between kernels on how `msghdr` is specified,
different libc projects such as musl and glibc provide different
workarounds by laying out `msghdr` differently amongst one another.

A few projects running tests/applications through QEMU have been bitten by
this, and a solution that one of the projects discovered was that patching
QEMU to zero-initialize the bytes msghdr the same way my patch does allow
for compatibility between different `msghdr` layouts across glibc, musl,
and the Linux kernel:
https://github.com/void-linux/void-packages/issues/23557#issuecomment-718392360

For some additional useful context, here's a link pointing changes musl
libc made to laying out `msghdr` b/c of Linux incorrectly laying out
`msghdr` against the POSIX standard:
http://git.musl-libc.org/cgit/musl/commit/?id=7168790763cdeb794df52be6e3b39fbb021c5a64

Also, here is a link to the `msghdr` struct layout in musl libc's bleeding
edge branch as of now:
https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h#n22

As for my rationale for sending in this patch, it is because I'm currently
implementing cross-platform networking in the standard library for the Zig
programming language, and have run into this exact same problem with
EMSGSIZE being returned by sendmsg() when tests are run through QEMU on
x86_64-linux-musl.

My discussions with the Zig community about it alongside debug logs
regarding the exact parameters being fed to the sendmsg() syscall can be
found here: https://github.com/ziglang/zig/pull/8750#issuecomment-841641576

Hope this gives enough context about the problem and patch, but please do
let me know if there is any more information that I could provide which
would help.

Best regards,
Kenta Iwasaki


On Sun, 16 May 2021 at 19:53, Laurent Vivier <laurent@vivier.eu> wrote:

> Le 16/05/2021 à 11:15, Kenta Iwasaki a écrit :
> > The mixing of libc and kernel versions of the layout of the `msghdr`
> > struct causes EMSGSIZE to be returned by sendmsg if the `msghdr` struct
> > is not zero-initialized (such that padding bytes comprise of
> > uninitialized memory).
> >
> > Other parts of the QEMU codebase appear to zero-initialize the `msghdr`
> > struct to workaround these struct layout issues, except for
> > do_sendrecvmsg_locked in linux-user/syscall.c.
> >
> > This patch zero-initializes the `msghdr` struct in
> > do_sendrecvmsg_locked.
> >
> > Signed-off-by: Kenta Iwasaki <kenta@lithdew.net>
> > ---
> >  linux-user/syscall.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 95d79ddc43..f60b7e04d5 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -3337,7 +3337,7 @@ static abi_long do_sendrecvmsg_locked(int fd,
> struct target_msghdr *msgp,
> >                                        int flags, int send)
> >  {
> >      abi_long ret, len;
> > -    struct msghdr msg;
> > +    struct msghdr msg = { 0 };
> >      abi_ulong count;
> >      struct iovec *vec;
> >      abi_ulong target_vec;
> >
>
> It seems do_sendrecvmsg_locked() initializes all the fields of the
> structure, I don't see why we
> need to clear it before use.
>
> Could you explain more?
>
> Thanks,
> Laurent
>

[-- Attachment #2: Type: text/html, Size: 5461 bytes --]