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

The recvmsg handling is more complicated because it requires check the
returned kernel control message and make some convertions.  For
!__ASSUME_TIME64_SYSCALLS it converts the first 32-bit time SO_TIMESTAMP
or SO_TIMESTAMPNS and appends it to the control buffer if has extra
space or returns MSG_CTRUNC otherwise.  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.

Checked on x86_64-linux-gnu and i686-linux-gnu (on 5.4 and on 4.15
kernel).

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>
---
 include/sys/socket.h                          |  5 +
 sysdeps/unix/sysv/linux/Makefile              |  2 +-
 sysdeps/unix/sysv/linux/Versions              |  1 +
 .../unix/sysv/linux/convert_scm_timestamps.c  | 96 +++++++++++++++++++
 sysdeps/unix/sysv/linux/getsockopt.c          | 12 +++
 .../sysv/linux/hppa/socket-constants-time64.h |  5 +
 .../sysv/linux/mips/socket-constants-time64.h |  5 +
 .../linux/powerpc/socket-constants-time64.h   |  5 +
 sysdeps/unix/sysv/linux/recvmsg.c             | 23 +++--
 sysdeps/unix/sysv/linux/setsockopt.c          | 12 +++
 .../unix/sysv/linux/socket-constants-time64.h |  5 +
 .../linux/sparc/socket-constants-time64.h     |  5 +
 12 files changed, 169 insertions(+), 7 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/convert_scm_timestamps.c

* 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>

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.

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.

Thanks,
Florian

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;
        }

* Adhemerval Zanella:

>> 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 ruby test case needs “set disable-randomization off” in GDB and even
then passes sporadically.  Without it, the test always succeeds for some
reason.

With the GCC 11 compiled binaries in Fedora, we fault in an isolated
error path (null pointer store followed by ud2 instruction).  I haven't
had time to look at the disassembly and GIMPLE dumps to see what is
going on.

>> 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).

Hmm.  I think it depends on previous buffer contents.

>> 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.

Yes, but the Fedora build does not use 64-bit time_t, so this conversion
is kind of pointless there.

I see that we didn't add a __recvmsg_time64 entrypoint (and similar
entrypoints for the other cases).  I think that's a mistake, we should
have those to future-proof things (similar for ioctl and fcntl—are there
any other multiplexers?).

> 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).

Yes, but old 32-bit binaries can avoid running the new code if we have
separate entrypoint.

> 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;
>         }

Sorry, I can't quite wrap my head around this right now.

Thanks,
Florian

On 25/06/2021 16:16, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> 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 ruby test case needs “set disable-randomization off” in GDB and even
> then passes sporadically.  Without it, the test always succeeds for some
> reason.
> 
> With the GCC 11 compiled binaries in Fedora, we fault in an isolated
> error path (null pointer store followed by ud2 instruction).  I haven't
> had time to look at the disassembly and GIMPLE dumps to see what is
> going on.

I will create a testcase with a different struct msghdr sizes and alignment,
along with different timestamps configurations.

> 
>>> 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).
> 
> Hmm.  I think it depends on previous buffer contents.

Ok, it might the case. But on my testing I also tried to add random 
initialization data on msghdr.

> 
>>> 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.
> 
> Yes, but the Fedora build does not use 64-bit time_t, so this conversion
> is kind of pointless there.> 
> I see that we didn't add a __recvmsg_time64 entrypoint (and similar
> entrypoints for the other cases).  I think that's a mistake, we should
> have those to future-proof things (similar for ioctl and fcntl—are there
> any other multiplexers?).
> 
>> 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).
> 
> Yes, but old 32-bit binaries can avoid running the new code if we have
> separate entrypoint.

There are two fold problems here:

  1. With __ASSUME_TIME64_SYSCALLS, the SO_* timestamp constants being used
     will always 64-bit.  It means we will need translation for programs
     built *without* _TIME_BITS=64.

  2. Without __ASSUME_TIME64_SYSCALLS, we can't know what kind of timestamp
     is being configured on the socket.  The setsockopt() will still maps
     to old 32-bit timestamps for ENOPROTOOPT.

A new 64-bit entrypoint does not help much in my opinion: we can't know
for sure if the socket has being configured with an old library or either
pass with 32-bit timestamps with SCM_RIGHTS.

> 
>> 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;
>>         }
> 
> Sorry, I can't quite wrap my head around this right now.
> 
> Thanks,
> Florian
>