Zero out the host's `msg_control` buffer

Commit Message

[e-mail] July 10, 2018, 11:32 p.m. UTC

(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(+)

Comments

Philippe Mathieu-Daudé July 11, 2018, 8:54 p.m. UTC | #1

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 <[e-mail]> (maintainer:Linux user)
Laurent Vivier <[e-mail]> (reviewer:Linux user)
[e-mail] (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é <[e-mail]>

Regards,

Phil.

Patch

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);