[PATCH] convert_scm_timestamps: Initialize buffer for CMSG_NXTHDR

[PATCH] convert_scm_timestamps: Initialize buffer for CMSG_NXTHDR

[Fabian Vogt]

The space past the msg_control buffer returned by the kernel needs to be
zero-initialized before CMSG_NXTHDR can be used. Currently this is not done,
and CMSG_NXTHDR reads the size of the uninitialized "future" cmsghdr inside.

Signed-off-by: Fabian Vogt <fvogt@suse.de>
---
Moin,

In openSUSE we also hit the issue that recvmsg returns invalid cmsg buffers,
manifesting itself as random segfaults in calls to "ping 127.0.0.1 -c2",
done by the testsuite of python-EasyProcess. I only found the relevant glibc
tickets after debugging myself, however that has the benefit that I can
confirm and add to the existing findings.

The biggest issue is the missing update of the "last" pointer in the case of
a non-SOL_SOCKET message, which is addressed properly by
[PATCH v3 2/2] linux: Fix ancillary 64-bit time timestamp conversion (BZ #28349, BZ #28350)

However, I also found another bug, which this patch addresses. Feel free to
incorporate it into the existing patch series or add it on top.

Adding a reliable testcase for this (if it's worth the effort at all) is not
trivial, ideally calling __convert_scm_timestamps with a manually crafted
cmsghdr past the old msg_controllen can be done. Otherwise the needed offset
for the crafted garbage depends on what recvmsg returns. Filling the buffer
with e.g. 0xFF or 0x01 (cmsg_len=0xFFFFFFFF or 0x01010101) causes the
calculation in CMSG_NXTHDR to overflow and so it thinks it fits into the buffer
still, so it requires more specific garbage to cause CMSG_NXTHDR to fail.

With both fixes applied, ping works reliably here and valgrind also stopped
complaining.

Cheers,
Fabian

diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
index 00c934c413..d0429b8353 100644
--- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
+++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
@@ -88,6 +88,8 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
       return;
     }
 
+  /* Zero memory for the new cmsghdr, required by CMSG_NXTHDR */
+  memset ((char *) (msg->msg_control) + msg->msg_controllen, 0, CMSG_SPACE (sizeof tvts));
   msg->msg_controllen += CMSG_SPACE (sizeof tvts);
   cmsg = CMSG_NXTHDR(msg, last);
   if (cmsg == NULL)

Re: [PATCH] convert_scm_timestamps: Initialize buffer for CMSG_NXTHDR

[Adhemerval Zanella]

On 15/12/2021 10:46, Fabian Vogt wrote:
> The space past the msg_control buffer returned by the kernel needs to be
> zero-initialized before CMSG_NXTHDR can be used. Currently this is not done,
> and CMSG_NXTHDR reads the size of the uninitialized "future" cmsghdr inside.
> 
> Signed-off-by: Fabian Vogt <fvogt@suse.de>
> ---
> Moin,
> 
> In openSUSE we also hit the issue that recvmsg returns invalid cmsg buffers,
> manifesting itself as random segfaults in calls to "ping 127.0.0.1 -c2",
> done by the testsuite of python-EasyProcess. I only found the relevant glibc
> tickets after debugging myself, however that has the benefit that I can
> confirm and add to the existing findings.
> 
> The biggest issue is the missing update of the "last" pointer in the case of
> a non-SOL_SOCKET message, which is addressed properly by
> [PATCH v3 2/2] linux: Fix ancillary 64-bit time timestamp conversion (BZ #28349, BZ #28350)
> 
> However, I also found another bug, which this patch addresses. Feel free to
> incorporate it into the existing patch series or add it on top.

Yes, it requires to avoid the UB of CMSG_NXTHDR as indicated by BZ#13500 [1].

> 
> Adding a reliable testcase for this (if it's worth the effort at all) is not
> trivial, ideally calling __convert_scm_timestamps with a manually crafted
> cmsghdr past the old msg_controllen can be done. Otherwise the needed offset
> for the crafted garbage depends on what recvmsg returns. Filling the buffer
> with e.g. 0xFF or 0x01 (cmsg_len=0xFFFFFFFF or 0x01010101) causes the
> calculation in CMSG_NXTHDR to overflow and so it thinks it fits into the buffer
> still, so it requires more specific garbage to cause CMSG_NXTHDR to fail.

Indeed I tried to reproduce it by using different value to fill the ancillary
data in my testcase but I could not actually trigger any issue.  However
valgrind does warn the use of initialize data.

> 
> With both fixes applied, ping works reliably here and valgrind also stopped
> complaining.

I have sent an updated from my patch with this fix incorporated [1].

[1] https://sourceware.org/pipermail/libc-alpha/2021-December/134556.html

> 
> Cheers,
> Fabian
> 
> diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> index 00c934c413..d0429b8353 100644
> --- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> +++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> @@ -88,6 +88,8 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>        return;
>      }
>  
> +  /* Zero memory for the new cmsghdr, required by CMSG_NXTHDR */
> +  memset ((char *) (msg->msg_control) + msg->msg_controllen, 0, CMSG_SPACE (sizeof tvts));

Line too long and there is no need to cast here (void arithmetic is a gcc
extension).

>    msg->msg_controllen += CMSG_SPACE (sizeof tvts);
>    cmsg = CMSG_NXTHDR(msg, last);
>    if (cmsg == NULL)
> 
> 

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=13500