Re: [PATCH v4] linux: Fix ancillary 64-bit time timestamp conversion (BZ #28349, BZ #28350)

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

On 21/01/2022 10:53, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> The __convert_scm_timestamps() only updates the control message last
>> pointer for SOL_SOCKET type, so if the message control buffer contains
>> multiple ancillary message types the converted timestamp one might
>> overwrite a valid message.
> 
> s/()/ function/?

Ack.

> 
>> The test check if the extra ancillary space is correctly handled
> 
> typo: the test check[s]

Ack.

> 
>> by recvmsg/recvmmsg, where if there is no extra space for the 64-bit
>> time_t converted message the control buffer should be marked with
>> MSG_TRUNC.  It also check if recvmsg/recvmmsg handle correctly multiple
>> ancillary data.
> 
> typo: MSG_[C]TRUNC
> 
> (I think this MSG_CTRUNC result is a remaining bug we need to fix
> separately for time32 mode.)

Do you mean not returning the MSG_CTRUNC?

> 
>> diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
>> index 00c934c413..36976c276f 100644
>> --- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
>> +++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
>> @@ -54,6 +54,8 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>>         cmsg != NULL;
>>         cmsg = CMSG_NXTHDR (msg, cmsg))
>>      {
>> +      last = cmsg;
>> +
>>        if (cmsg->cmsg_level != SOL_SOCKET)
>>  	continue;
>>  
>> @@ -75,8 +77,6 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>>  	  tvts[1] = tmp[1];
>>  	  break;
>>  	}
>> -
>> -      last = cmsg;
>>      }
>>  
>>    if (last == NULL || type == 0)
> 
> I think the last == NULL check is now redundant.  It's probably clearer
> to remove it.

Indeed.

> 
>> @@ -88,10 +88,11 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>>        return;
>>      }
> 
> (The necessary length check is not visible in the patch, but it is
> there.)
> 
>> +  /* Zero memory for the new cmsghdr, required by CMSG_NXTHDR.  */
> 
> This is specifically about the cmsg_len field, right?  Maybe mention
> this in the comment.

I changed to

  /* Zero memory for the new cmsghdr, so reading cmsg_len field
     by CMSG_NXTHDR does not trigger UB.  */

> 
>> +  memset (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)
>> -    return;
>> +  cmsg = CMSG_NXTHDR (msg, last);
>>    cmsg->cmsg_level = SOL_SOCKET;
>>    cmsg->cmsg_type = type;
>>    cmsg->cmsg_len = CMSG_LEN (sizeof tvts);
> 
> CMSG_NXTHDR cannot be NULL anymore because of the previous length check
> and cmsg_len set to zero.  Okay.
> 
>> diff --git a/sysdeps/unix/sysv/linux/tst-socket-timestamp.c b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
>> new file mode 100644
>> index 0000000000..3854c46bad
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
>> @@ -0,0 +1,344 @@
> 
>> +/* Some extra space added for ancillary data, it might be used to convert
>> +   32-bit timestamp to 64-bit for _TIME_BITS=64.  */
>> +enum { slack_max_size = 64 };
>> +static const int slack[] = { 0, 4, 8, 16, 32, slack_max_size };
> 
> I was worried whether 4 is okay here, but we read the cmsg buffer via
> memcpy, so we always have sufficient alignment.
> 
>> +
>> +static bool support_64_timestamp;
>> +/* AF_INET socket and address used to send and receive data.  */
>> +static int srv;
>> +static struct sockaddr_in srv_addr;
> 
> I think the comment should mention receiving only.

Ack.

> 
>> +
>> +static int
>> +do_sendto (const struct sockaddr_in *addr, int nmsgs)
>> +{
>> +  int s = xsocket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
>> +  xconnect (s, (const struct sockaddr *) addr, sizeof (*addr));
>> +
>> +  for (int i = 0; i < nmsgs; i++)
>> +    xsendto (s, &i, sizeof (i), 0, (const struct sockaddr *) addr,
>> +	     sizeof (*addr));
>> +
>> +  return 0;
>> +}
> 
> Missing xclose (s).

Ack.

> 
>> +static void
>> +do_recvmsg_slack_ancillary (bool use_multi_call, int s, void *cmsg,
>> +			    size_t slack, size_t tsize, int exp_msg)
>> +{
>> +  int msg;
> 
> (This is actually the payload.)
> 
>> +  /* If there is not timestamp in the ancilliary data, recvmsg should set
>> +     the flag inidcating it.  */
> 
> typo: in[di]cating

Ack.

> 
>> +  if (exp_timestamp && !timestamp)
>> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);
> 
> Shouldn't this be MSG_CTRUNC?  I expect this is actually dead code.

Indeed, I think this is a leftover when I was writing the patch. I will remove
it.

> 
>> +static void
>> +do_test_slack_space (void)
>> +{
>> +  /* Setup the ancillary data buffer with an extra page with PROT_NONE to
>> +     check the possible timestamp conversion on some systems.  */
>> +  struct support_next_to_fault nf =
>> +    support_next_to_fault_allocate (slack_max_size);
>> +  void *msgbuf = nf.buffer + slack_max_size;
>> +
>> +  /* Enable the timestamp using struct timeval precision.  */
>> +  {
>> +    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMP, &(int){1},
>> +			sizeof (int));
>> +    TEST_VERIFY_EXIT (r != -1);
>> +  }
>> +  /* Check recvmsg.  */
>> +  do_sendto (&srv_addr, array_length (slack));
>> +  for (int s = 0; s < array_length (slack); s++) {
> 
> style: { should be on its own line (repeated below).

Ack.

> 
>> +/* Check if the converted 64-bit timestamp is correctly appended when there
>> +   are multiple ancillary messages.  */
>> +static void
>> +do_recvmsg_multiple_ancillary (bool use_multi_call, int s, void *cmsg,
>> +			       size_t cmsgsize, int exp_msg)
>> +{
> 
>> +  /* If there is no timestamp in the ancillary data, recvmsg should set
>> +     the flag to indicate it.  */
>> +  if (!timestamp)
>> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);
> 
> MSG_CTRUNC? (see above)

Ack, I will remove it.