Message ID | 7960950.1uK8C5oMiK@linux-e202.suse.de |
---|---|
State | New |
Headers | show |
Series | convert_scm_timestamps: Initialize buffer for CMSG_NXTHDR | expand |
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
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)
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