| Message ID | 20210516091536.1042693-1-kenta@lithdew.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user/syscall: zero-init msghdr in do_sendrecvmsg_locked | expand |
Doing a ping for this patch. https://patchew.org/QEMU/20210516091536.1042693-1-kenta@lithdew.net/ Best regards, Kenta Iwasaki On Sun, 16 May 2021 at 21:57, Kenta Iwasaki <kenta@lithdew.net> wrote: > 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 >> >
Le 16/05/2021 à 14:57, Kenta Iwasaki a écrit : > 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 > <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 > <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 > <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 > <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. Thank you for the explanation and sorry for the delay. As we use directly the kernel syscall rather than the libc wrapper we should not use here the msghdr definition from the libc but the one from the kernel. The one we receive is also the one from the kernel as we trap the kernel syscall not the libc call. So the code should be libc-agnostic... The reference implementation in our case is 'struct user_msghdr' from the kernel, and we need to duplicate it in QEMU to be able to use it. Thanks, LAurent
No worries, though I would also like to apologize as I find that the explanation I gave in my last email was a little all over the place. To clarify my last e-mail, I believe the current msghdr struct layout in QEMU is libc-agnostic, but not kernel agnostic. Rather, the current msghdr struct layout defined in QEMU follows the definition of the Linux kernel's own struct layout of msghdr, which isn't compliant with POSIX. Zeroing the bytes in the patch I provided, which is a trick used in musl libc as well to be both kernel-agnostic and libc-agnostic, would make QEMU's struct layout definition of msghdr be both kernel-agnostic to all kernels adhering to the POSIX standard, and libc-agnostic as well. Best regards, Kenta Iwasaki On Sun, 20 Jun 2021 at 23:56, Laurent Vivier <laurent@vivier.eu> wrote: > Le 16/05/2021 à 14:57, Kenta Iwasaki a écrit : > > 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 > > < > 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 > > < > 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 > > <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 > > <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. > > Thank you for the explanation and sorry for the delay. > > As we use directly the kernel syscall rather than the libc wrapper we > should not use here the msghdr > definition from the libc but the one from the kernel. > > The one we receive is also the one from the kernel as we trap the kernel > syscall not the libc call. > > So the code should be libc-agnostic... > > The reference implementation in our case is 'struct user_msghdr' from the > kernel, and we need to > duplicate it in QEMU to be able to use it. > > Thanks, > LAurent > >
Le 20/06/2021 à 17:09, Kenta Iwasaki a écrit : > No worries, though I would also like to apologize as I find that the explanation I gave in my last > email was a little all over the place. > > To clarify my last e-mail, I believe the current msghdr struct layout in QEMU is libc-agnostic, but > not kernel agnostic. Rather, the current msghdr struct layout defined in QEMU follows the definition > of the Linux kernel's own struct layout of msghdr, which isn't compliant with POSIX. > > Zeroing the bytes in the patch I provided, which is a trick used in musl libc as well to be both > kernel-agnostic and libc-agnostic, would make QEMU's struct layout definition of msghdr be both > kernel-agnostic to all kernels adhering to the POSIX standard, and libc-agnostic as well. Yes, linux-user is not kernel agnostic. It's why I would prefer to define a "host_msghdr" and use it as a parameter to the host outgoing syscall (we already define "target_msghdr" for target incoming syscall). host_msghdr would be a copy of kernel user_msghdr. But what do you mean by "all kernels": do we have linux kernels that use a different msghdr definition (POSIX one)? Thanks, Laurent > Best regards, > Kenta Iwasaki > > On Sun, 20 Jun 2021 at 23:56, Laurent Vivier <laurent@vivier.eu <mailto:laurent@vivier.eu>> wrote: > > Le 16/05/2021 à 14:57, Kenta Iwasaki a écrit : > > 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 > <https://github.com/void-linux/void-packages/issues/23557#issuecomment-718392360> > > <https://github.com/void-linux/void-packages/issues/23557#issuecomment-718392360 > <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 > <http://git.musl-libc.org/cgit/musl/commit/?id=7168790763cdeb794df52be6e3b39fbb021c5a64> > > <http://git.musl-libc.org/cgit/musl/commit/?id=7168790763cdeb794df52be6e3b39fbb021c5a64 > <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 > <https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h#n22> > > <https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h#n22 > <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 > <https://github.com/ziglang/zig/pull/8750#issuecomment-841641576> > > <https://github.com/ziglang/zig/pull/8750#issuecomment-841641576 > <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. > > Thank you for the explanation and sorry for the delay. > > As we use directly the kernel syscall rather than the libc wrapper we should not use here the msghdr > definition from the libc but the one from the kernel. > > The one we receive is also the one from the kernel as we trap the kernel syscall not the libc call. > > So the code should be libc-agnostic... > > The reference implementation in our case is 'struct user_msghdr' from the kernel, and we need to > duplicate it in QEMU to be able to use it. > > Thanks, > LAurent >
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;
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(-)