This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: BZ# 16418: Fix powerpc get_clockfreq raciness


Ping.


On 10-12-2014 15:18, Adhemerval Zanella wrote:
> Ping.
>
> On 24-11-2014 19:44, Adhemerval Zanella wrote:
>> This patch fixes powerpc __get_clockfreq racy and cancel-safe issues by
>> dropping internal static cache and by using nocancel file operations.
>> The vDSO failure check is also removed, since kernel code does not
>> return an error (it cleans cr0.so bit on function return) and the static
>> code (to read value /proc) now uses non-cancellable calls.
>>
>> Since currently I don't see this code patch to be performance sensitive
>> (usually the clock frequency is obtained once to transform timebase
>> values), I don't see a problem to drop its internal cache.  Also, if
>> latency came up as being important for this one, correct approach would
>> be use IFUNC to call vDSO symbols directly (which I do not aim to
>> implement now).
>>
>> Tested on powerpc64 and powerpc32.
>>
>> --
>>
>> 	[BZ# 16418]
>> 	* sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>> 	(__get_clockfreq): Make code racy and cancel safe.
>>
>> ---
>>
>> diff --git a/NEWS b/NEWS
>> index ad170c4..833a680 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -9,10 +9,10 @@ Version 2.21
>>
>>  * The following bugs are resolved with this release:
>>
>> -  6652, 12926, 14132, 14138, 14171, 14498, 15215, 15884, 16469, 17266,
>> -  17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501, 17506,
>> -  17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574, 17582, 17583,
>> -  17584, 17585, 17589, 17594, 17616, 17625.
>> +  6652, 12926, 14132, 14138, 14171, 14498, 15215, 15884, 16418, 16469,
>> +  17266, 17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501,
>> +  17506, 17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574, 17582,
>> +  17583, 17584, 17585, 17589, 17594, 17616, 17625.
>>
>>  * CVE-2104-7817 The wordexp function could ignore the WRDE_NOCMD flag
>>    under certain input conditions resulting in the execution of a shell for
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>> index 62217b1..44f90b4 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>> +++ b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>> @@ -24,95 +24,85 @@
>>  #include <libc-internal.h>
>>  #include <sysdep.h>
>>  #include <bits/libc-vdso.h>
>> +#include <not-cancel.h>
>>
>>  hp_timing_t
>>  __get_clockfreq (void)
>>  {
>> +  hp_timing_t result = 0L;
>> +
>> +#ifdef SHARED
>> +  /* The vDSO does not return an error (it clear cr0.so on returning).  */
>> +  INTERNAL_SYSCALL_DECL (err);
>> +  result =
>> +    INTERNAL_VSYSCALL_NO_SYSCALL_FALLBACK (get_tbfreq, err, uint64_t, 0);
>> +#else
>>    /* We read the information from the /proc filesystem.  /proc/cpuinfo
>>       contains at least one line like:
>>       timebase        : 33333333
>>       We search for this line and convert the number into an integer.  */
>> -  static hp_timing_t timebase_freq;
>> -  hp_timing_t result = 0L;
>> +  int fd = __open_nocancel ("/proc/cpuinfo", O_RDONLY);
>> +  if (__glibc_likely (fd != -1))
>> +    return result;
>>
>> -  /* If this function was called before, we know the result.  */
>> -  if (timebase_freq != 0)
>> -    return timebase_freq;
>> +  /* The timebase will be in the 1st 1024 bytes for systems with up
>> +     to 8 processors.  If the first read returns less then 1024
>> +     bytes read,  we have the whole cpuinfo and can start the scan.
>> +     Otherwise we will have to read more to insure we have the
>> +     timebase value in the scan.  */
>> +  char buf[1024];
>> +  ssize_t n;
>>
>> -  /* If we can use the vDSO to obtain the timebase even better.  */
>> -#ifdef SHARED
>> -  INTERNAL_SYSCALL_DECL (err);
>> -  timebase_freq =
>> -    INTERNAL_VSYSCALL_NO_SYSCALL_FALLBACK (get_tbfreq, err, uint64_t, 0);
>> -  if (INTERNAL_SYSCALL_ERROR_P (timebase_freq, err)
>> -      && INTERNAL_SYSCALL_ERRNO (timebase_freq, err) == ENOSYS)
>> -#endif
>> +  n = __read_nocancel (fd, buf, sizeof (buf));
>> +  if (n == sizeof (buf))
>>      {
>> -      int fd = __open ("/proc/cpuinfo", O_RDONLY);
>> +      /* We are here because the 1st read returned exactly sizeof
>> +         (buf) bytes.  This implies that we are not at EOF and may
>> +         not have read the timebase value yet.  So we need to read
>> +         more bytes until we know we have EOF.  We copy the lower
>> +         half of buf to the upper half and read sizeof (buf)/2
>> +         bytes into the lower half of buf and repeat until we
>> +         reach EOF.  We can assume that the timebase will be in
>> +         the last 512 bytes of cpuinfo, so two 512 byte half_bufs
>> +         will be sufficient to contain the timebase and will
>> +         handle the case where the timebase spans the half_buf
>> +         boundry.  */
>> +      const ssize_t half_buf = sizeof (buf) / 2;
>> +      while (n >= half_buf)
>> +	{
>> +	  memcpy (buf, buf + half_buf, half_buf);
>> +	  n = __read_nocancel (fd, buf + half_buf, half_buf);
>> +	}
>> +      if (n >= 0)
>> +	n += half_buf;
>> +    }
>> +  __close_nocancel (fd);
>>
>> -      if (__glibc_likely (fd != -1))
>> +  if (__glibc_likely (n > 0))
>> +    {
>> +      char *mhz = memmem (buf, n, "timebase", 7);
>> +
>> +      if (__glibc_likely (mhz != NULL))
>>  	{
>> -	  /* The timebase will be in the 1st 1024 bytes for systems with up
>> -	     to 8 processors.  If the first read returns less then 1024
>> -	     bytes read,  we have the whole cpuinfo and can start the scan.
>> -	     Otherwise we will have to read more to insure we have the
>> -	     timebase value in the scan.  */
>> -	  char buf[1024];
>> -	  ssize_t n;
>> +	  char *endp = buf + n;
>>
>> -	  n = __read (fd, buf, sizeof (buf));
>> -	  if (n == sizeof (buf))
>> -	    {
>> -	      /* We are here because the 1st read returned exactly sizeof
>> -	         (buf) bytes.  This implies that we are not at EOF and may
>> -	         not have read the timebase value yet.  So we need to read
>> -	         more bytes until we know we have EOF.  We copy the lower
>> -	         half of buf to the upper half and read sizeof (buf)/2
>> -	         bytes into the lower half of buf and repeat until we
>> -	         reach EOF.  We can assume that the timebase will be in
>> -	         the last 512 bytes of cpuinfo, so two 512 byte half_bufs
>> -	         will be sufficient to contain the timebase and will
>> -	         handle the case where the timebase spans the half_buf
>> -	         boundry.  */
>> -	      const ssize_t half_buf = sizeof (buf) / 2;
>> -	      while (n >= half_buf)
>> -		{
>> -		  memcpy (buf, buf + half_buf, half_buf);
>> -		  n = __read (fd, buf + half_buf, half_buf);
>> -		}
>> -	      if (n >= 0)
>> -		n += half_buf;
>> -	    }
>> +	  /* Search for the beginning of the string.  */
>> +	  while (mhz < endp && (*mhz < '0' || *mhz > '9') && *mhz != '\n')
>> +	    ++mhz;
>>
>> -	  if (__builtin_expect (n, 1) > 0)
>> +	  while (mhz < endp && *mhz != '\n')
>>  	    {
>> -	      char *mhz = memmem (buf, n, "timebase", 7);
>> -
>> -	      if (__glibc_likely (mhz != NULL))
>> +	      if (*mhz >= '0' && *mhz <= '9')
>>  		{
>> -		  char *endp = buf + n;
>> -
>> -		  /* Search for the beginning of the string.  */
>> -		  while (mhz < endp && (*mhz < '0' || *mhz > '9')
>> -			 && *mhz != '\n')
>> -		    ++mhz;
>> -
>> -		  while (mhz < endp && *mhz != '\n')
>> -		    {
>> -		      if (*mhz >= '0' && *mhz <= '9')
>> -			{
>> -			  result *= 10;
>> -			  result += *mhz - '0';
>> -			}
>> -
>> -		      ++mhz;
>> -		    }
>> +		  result *= 10;
>> +		  result += *mhz - '0';
>>  		}
>> -	      timebase_freq = result;
>> +
>> +	      ++mhz;
>>  	    }
>> -	  __close (fd);
>>  	}
>>      }
>> +#endif
>>
>> -  return timebase_freq;
>> +  return result;
>>  }
>>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]