1. Patchew
  2. QEMU
  3. View series

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

Jonas Schievink posted 1 patch 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180710233229.9311-1-jonasschievink@gmail.com
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
There is a newer version of this series
linux-user/syscall.c | 2 ++
1 file changed, 2 insertions(+)
[Qemu-devel] [PATCH] Zero out the host's `msg_control` buffer
Posted by Jonas Schievink 5 years, 9 months ago 
(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(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e4b1b7d7da..77ce173b27 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3845,6 +3845,8 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
     msg.msg_control = alloca(msg.msg_controllen);
     msg.msg_flags = tswap32(msgp->msg_flags);
 
+    memset(msg.msg_control, 0, msg.msg_controllen);
+
     count = tswapal(msgp->msg_iovlen);
     target_vec = tswapal(msgp->msg_iov);
 
-- 
2.18.0
Re: [Qemu-devel] [PATCH] Zero out the host's `msg_control` buffer
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago 
Hi Jonas,

You forgot to notify the maintainers, see
https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer :

./scripts/get_maintainer.pl -f linux-user/syscall.c
Riku Voipio <riku.voipio@iki.fi> (maintainer:Linux user)
Laurent Vivier <laurent@vivier.eu> (reviewer:Linux user)
qemu-devel@nongnu.org (open list:All patches CC here)

On 07/10/2018 08:32 PM, Jonas Schievink wrote:
> (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).

This misses your Signed-off-by:

https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_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..77ce173b27 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3845,6 +3845,8 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
>      msg.msg_control = alloca(msg.msg_controllen);
>      msg.msg_flags = tswap32(msgp->msg_flags);
>  
> +    memset(msg.msg_control, 0, msg.msg_controllen);
> +
>      count = tswapal(msgp->msg_iovlen);
>      target_vec = tswapal(msgp->msg_iov);

Do you mind swapping:

>      msg.msg_control = alloca(msg.msg_controllen);
> +    memset(msg.msg_control, 0, msg.msg_controllen);
> +
>      msg.msg_flags = tswap32(msgp->msg_flags);

With your Signed-off-by:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Regards,

Phil.
[Qemu-devel] [PATCH v2] Zero out the host's `msg_control` buffer
Posted by Jonas Schievink 5 years, 9 months 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 5 years, 9 months 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 5 years, 9 months 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 5 years, 9 months 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
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.