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

[Laurent Vivier]

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 <address@hidden>

> On Thu, Jul 12, 2018 at 6:05 PM Laurent Vivier <address@hidden
> <mailto:address@hidden>> 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 <address@hidden
>     <mailto:address@hidden>>
>     > ---
>     > 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
>