| Message ID | 20180711221244.31869-1-jonasschievink@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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 >
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);
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(+)