[Qemu-devel] [PATCH v2] Zero out the host's `msg_control` buffer

Jonas Schievink posted 1 patch 10 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180711221244.31869-1-jonasschievink@gmail.com
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
linux-user/syscall.c | 2 ++
1 file changed, 2 insertions(+)

[Qemu-devel] [PATCH v2] Zero out the host's `msg_control` buffer

Posted by Jonas Schievink 10 weeks ago
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 <jonasschievink@gmail.com>
---
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);
+
     msg.msg_flags = tswap32(msgp->msg_flags);
 
     count = tswapal(msgp->msg_iovlen);
-- 
2.18.0


Re: [Qemu-devel] [PATCH v2] Zero out the host's `msg_control` buffer

Posted by Laurent Vivier 10 weeks ago
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 <jonasschievink@gmail.com>
> ---
> 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

Re: [Qemu-devel] [PATCH v2] Zero out the host's `msg_control` buffer

Posted by Jonas Schievink 10 weeks ago
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.

Regards,
Jonas

On Thu, Jul 12, 2018 at 6:05 PM Laurent Vivier <laurent@vivier.eu> 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 <jonasschievink@gmail.com>
> > ---
> > 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
>

Re: [Qemu-devel] [PATCH v2] Zero out the host's `msg_control` buffer

Posted by Laurent Vivier 10 weeks ago
Le 12/07/2018 à 20:22, Jonas Schievink a écrit :
> 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.

ok, thank you for the advanced explanation.

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> On Thu, Jul 12, 2018 at 6:05 PM Laurent Vivier <laurent@vivier.eu
> <mailto:laurent@vivier.eu>> 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 <jonasschievink@gmail.com
>     <mailto:jonasschievink@gmail.com>>
>     > ---
>     > 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
>