Re: [PATCH v3 05/24] linux: Add fallback for 64-bit time_t SO_TIMESTAMP{NS}

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 25/06/2021 12:20, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> +/* It converts the first SO_TIMESTAMP or SO_TIMESTAMPNS with 32-bit time and
>> +   appends it to the control buffer.  The 32-bit time field is kept as-is.
>> +
>> +   Calls with __TIMESIZE=32 will see the converted 64-bit time control
>> +   messages as spurious control message of unknown type.
>> +
>> +   Calls with __TIMESIZE=64 running on pre-time64 kernels will see the
>> +   original message as a spurious control ones of unknown typ while running
>> +   on kernel with native 64-bit time support will only see the time64 version
>> +   of the control message.  */
>> +void
>> +__convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>> +{
>> +  if (msg->msg_control == NULL || msg->msg_controllen == 0)
>> +    return;
>> +
>> +  /* The returned control message format for SO_TIMESTAMP_NEW is a
>> +     'struct __kernel_sock_timeval' while for SO_TIMESTAMPNS_NEW is a
>> +     'struct __kernel_timespec'.  In either case it is two uint64_t
>> +     members.  */
>> +  uint64_t tvts[2];
>> +
>> +  struct cmsghdr *cmsg, *last = NULL;
>> +  int type = 0;
>> +
>> +  for (cmsg = CMSG_FIRSTHDR (msg);
>> +       cmsg != NULL;
>> +       cmsg = CMSG_NXTHDR (msg, cmsg))
>> +    {
>> +      if (cmsg->cmsg_level != SOL_SOCKET)
>> +	continue;
>> +
>> +      switch (cmsg->cmsg_type)
>> +	{
>> +	case COMPAT_SO_TIMESTAMP_OLD:
>> +	  if (type != 0)
>> +	    break;
>> +	  type = COMPAT_SO_TIMESTAMP_NEW;
>> +	  goto common;
>> +
>> +	case COMPAT_SO_TIMESTAMPNS_OLD:
>> +	  type = COMPAT_SO_TIMESTAMPNS_NEW;
>> +
>> +	/* fallthrough  */
>> +	common:
>> +	  memcpy (tvts, CMSG_DATA (cmsg), sizeof (tvts));
>> +	  break;
>> +	}
>> +
>> +      last = cmsg;
>> +    }
>> +
>> +  if (last == NULL || type == 0)
>> +    return;
>> +
>> +  if (CMSG_SPACE (sizeof tvts) > msgsize - msg->msg_controllen)
>> +    {
>> +      msg->msg_flags |= MSG_CTRUNC;
>> +      return;
>> +    }
>> +
>> +  msg->msg_controllen += CMSG_SPACE (sizeof tvts);
>> +  cmsg = CMSG_NXTHDR(msg, last);
>> +  cmsg->cmsg_level = SOL_SOCKET;
>> +  cmsg->cmsg_type = type;
>> +  cmsg->cmsg_len = CMSG_LEN (sizeof tvts);
>> +  memcpy (CMSG_DATA (cmsg), tvts, sizeof tvts);
>> +}
>> +libc_hidden_def (__convert_scm_timestamps)
>> +#endif
> 
> The Ruby test suite crashes on this line:
> 
>   cmsg->cmsg_level = SOL_SOCKET;
> 
> See:
> 
>   ruby: FTBFS with test suite failure (glibc 2.34 related)
>   <https://bugzilla.redhat.com/show_bug.cgi?id=1975144>

Do we have a more contained testcase? I am trying to trigger using the ruby
example it is kind hard to no make it use the system libraries.

I am trying to create a testcase with different cmsghdr sizes, but at least 
on i686 I can't really reproduce the issue (I am also running on 5.11 kernel).

> 
> The disassembly suggests that GCC has detected some undefined behavior.
> 
> This looks like a related bug:
> 
>   __cmsg_nxthdr in cmsg_nxthdr.c (CMSG_NXTHDR) has undefined behavior when setting up ancillary data
>   <https://sourceware.org/bugzilla/show_bug.cgi?id=13500>
> 
> I believe you cannot use CMSG_NXTHDR to append data in this way.

I think I am getting luck here because the example provided does pass,
even when tested against valgrind, asan, and ubsan (using gcc 10).

> 
> The other question is why this code is running at all.  Doing this
> complex conversion for a 32-bit applications doing a 32-bit function
> call on a kernel which supports 32-bit system calls does not make much
> sense to me.

Mainly because kernel does not provide a 64-bit recvmsg, different than
recvmmg.  So for 64-bit time_t calls we need to do the conversion,
although it won't help much if the caller does not provide a buffer large
enough.

I don't think we can improve it much by adding a 64-bit symbol: the
underlying syscall will be the same we don't prior hand which
SO_TIMESTAMP value were used to setup the timer (32-bit or 64-bit
one).

Revising the code I found one issue with __convert_scm_timestamps,
where the memcpy might indeed being accessing invalid memory:

diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
index d75a4618dd..2c61267fec 100644
--- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
+++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
@@ -44,7 +44,8 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
      'struct __kernel_sock_timeval' while for SO_TIMESTAMPNS_NEW is a
      'struct __kernel_timespec'.  In either case it is two uint64_t
      members.  */
-  uint64_t tvts[2];
+  int64_t tvts[2];
+  int32_t tmp;
 
   struct cmsghdr *cmsg, *last = NULL;
   int type = 0;
@@ -69,7 +70,10 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
 
        /* fallthrough  */
        common:
-         memcpy (tvts, CMSG_DATA (cmsg), sizeof (tvts));
+         memcpy (&tmp, CMSG_DATA (cmsg), sizeof (tmp));
+         tvts[0] = tmp;
+         memcpy (&tmp, CMSG_DATA (cmsg) + sizeof (tmp), sizeof (tmp));
+         tvts[1] = tmp;
          break;
        }