convert_scm_timestamps: Initialize buffer for CMSG_NXTHDR
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
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
Comments
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
@@ -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)