Zero out the host's `msg_control` buffer

(Apparently I messed up my git config for the last email so it didn't send the correct name - please bear with me, this is my first time submitting a patch to a mailing list. I've also added a link to the upstream bug in the commit description.)

If this is not done, qemu would drop any control message after the first one.

This is because glibc's CMSG_NXTHDR macro accesses the uninitialized cmsghdr's length field in order to find out if the message fits into the msg_control buffer, wrongly assuming that it doesn't because the length field contains garbage. Accessing the length field is fine for completed messages we receive from the kernel, but is - as far as I know
- not needed since the kernel won't return such an invalid cmsghdr in the first place.

This is tracked as this glibc bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=13500

It's probably also a good idea to bail with an error if CMSG_NXTHDR returns NULL but TARGET_CMSG_NXTHDR doesn't (ie. we still expect cmsgs).

---
 linux-user/syscall.c | 2 ++
 1 file changed, 2 insertions(+)

linux-user/syscall.c +0 -0

index e4b1b7d7da..77ce173b27 100644

If this is not done, qemu would drop any control message after the first one.

This is because glibc's CMSG_NXTHDR macro accesses the uninitialized cmsghdr's length field in order to find out if the message fits into the msg_control buffer, wrongly assuming that it doesn't because the length field contains garbage. Accessing the length field is fine for completed messages we receive from the kernel, but is - as far as I know
- not needed since the kernel won't return such an invalid cmsghdr in the first place.

This is tracked as this glibc bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=13500

It's probably also a good idea to bail with an error if CMSG_NXTHDR returns NULL but TARGET_CMSG_NXTHDR doesn't (ie. we still expect cmsgs).

Signed-off-by Jonas Schievink <[email protected]>

---
Changes in v2:
- put the memset right after the msg_control alloca
- added missing Signed-off-by line

 linux-user/syscall.c | 2 ++
 1 file changed, 2 insertions(+)

linux-user/syscall.c +0 -0

index e4b1b7d7da..3c427500ef 100644

linux-user/syscall.c static abi_long do_sendrecvmsg_locked(int fd,

Quoted 32 lines · Jonas Schievink

If this is not done, qemu would drop any control message after the first one.

This is because glibc's CMSG_NXTHDR macro accesses the uninitialized cmsghdr's length field in order to find out if the message fits into the msg_control buffer, wrongly assuming that it doesn't because the length field contains garbage. Accessing the length field is fine for completed messages we receive from the kernel, but is - as far as I know
- not needed since the kernel won't return such an invalid cmsghdr in the first place.

This is tracked as this glibc bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=13500

It's probably also a good idea to bail with an error if CMSG_NXTHDR returns NULL but TARGET_CMSG_NXTHDR doesn't (ie. we still expect cmsgs).

Signed-off-by Jonas Schievink <[email protected]>

---
Changes in v2:
- put the memset right after the msg_control alloca
- added missing Signed-off-by line

 linux-user/syscall.c | 2 ++
 1 file changed, 2 insertions(+)

linux-user/syscall.c +0 -0

index e4b1b7d7da..3c427500ef 100644
@@ -3843,6 +3843,8 @@ static abi_long do_sendrecvmsg_locked(int fd,

struct target_msghdr *msgp,

linux-user/syscall.c static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp, · line 3847

Quoted 5 lines

linux-user/syscall.c +2 -0

     }
     msg.msg_controllen = 2 * tswapal(msgp->msg_controllen);
     msg.msg_control = alloca(msg.msg_controllen);
+    memset(msg.msg_control, 0, msg.msg_controllen);
+

I'm not sure it is needed as the content of msg.control will be overwritten by target_to_host_cmsg() from the content of msgp->control.

Do you have a test case revealing the bug?

Thanks,
Laurent

Yes, I do.
See https://gist.github.com/jonas-schievink/cb6e6584a055539d2113f22d91068e2d

The problem is that glibc's CMSG_NXTHDR macro will access the header of the *next* message which isn't yet overwritten by QEMU, so it still contains garbage at that point. In particular, it will access the length field of the header to check if the message fits inside the msg_control buffer. If the length contains garbage, it may think that it doesn't fit and returns NULL. This is also mentioned in the glibc bug report I linked in the previous mail
(https://sourceware.org/bugzilla/show_bug.cgi?id=13500), and the memset workaround is mentioned there as well.

On Thu, Jul 12, 2018 at 6:05 PM Laurent Vivier <[email protected] <mailto:[email protected]>> wrote:

    Le 12/07/2018 à 00:12, Jonas Schievink a écrit :
    > If this is not done, qemu would drop any control message after the
    first
    > one.
    >
    > This is because glibc's `CMSG_NXTHDR` macro accesses the uninitialized
    > cmsghdr's length field in order to find out if the message fits
    into the
    > `msg_control` buffer, wrongly assuming that it doesn't because the
    > length field contains garbage. Accessing the length field is fine for
    > completed messages we receive from the kernel, but is - as far as
    I know
    > - not needed since the kernel won't return such an invalid cmsghdr in
    > the first place.
    >
    > This is tracked as this glibc bug:
    > https://sourceware.org/bugzilla/show_bug.cgi?id=13500
    >
    > It's probably also a good idea to bail with an error if `CMSG_NXTHDR`
    > returns NULL but `TARGET_CMSG_NXTHDR` doesn't (ie. we still expect
    > cmsgs).
    >
    > Signed-off-by: Jonas Schievink <[email protected]
    <mailto:[email protected]>>
    > ---
    > Changes in v2:
    > - put the memset right after the msg_control alloca
    > - added missing Signed-off-by line
    >
    >  linux-user/syscall.c | 2 ++
    >  1 file changed, 2 insertions(+)
    >
    > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
    > index e4b1b7d7da..3c427500ef 100644
    > --- a/linux-user/syscall.c
    > +++ b/linux-user/syscall.c
    > @@ -3843,6 +3843,8 @@ static abi_long do_sendrecvmsg_locked(int
    fd, struct target_msghdr *msgp,
    >      }
    >      msg.msg_controllen = 2 * tswapal(msgp->msg_controllen);
    >      msg.msg_control = alloca(msg.msg_controllen);
    > +    memset(msg.msg_control, 0, msg.msg_controllen);
    > +

    I'm not sure it is needed as the content of msg.control will be
    overwritten by target_to_host_cmsg() from the content of msgp->control.

    Do you have a test case revealing the bug?

    Thanks,
    Laurent