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