Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

[Florian Weimer]

Currently, __fxstat may get mapped to the fstat system call, while fstat
goes to fstatat.  This makes sandboxing issues less obvious to debug:

  <https://bugs.chromium.org/p/chromium/issues/detail?id=1164975>

Should we change this before the release?  And if yes, in what
direction?

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

[Adhemerval Zanella]

On 14/01/2021 09:00, Florian Weimer via Libc-alpha wrote:
> Currently, __fxstat may get mapped to the fstat system call, while fstat
> goes to fstatat.  This makes sandboxing issues less obvious to debug:
> 
>   <https://bugs.chromium.org/p/chromium/issues/detail?id=1164975>
> 
> Should we change this before the release?  And if yes, in what
> direction?

Implementing fstat/lstat/stat through fstatat does really simplify
the code at *lot* for some architectures. And on others and on some
ABI (32-bit with 64-bit time_t), it would require to use the same
syscall anyway (statx).

I don't really see any gain in adding back this complexity back on
stat calls, specially with y2038 support requirement.  

Re: Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> On 14/01/2021 09:00, Florian Weimer via Libc-alpha wrote:
>> Currently, __fxstat may get mapped to the fstat system call, while fstat
>> goes to fstatat.  This makes sandboxing issues less obvious to debug:
>> 
>>   <https://bugs.chromium.org/p/chromium/issues/detail?id=1164975>
>> 
>> Should we change this before the release?  And if yes, in what
>> direction?
>
> Implementing fstat/lstat/stat through fstatat does really simplify
> the code at *lot* for some architectures. And on others and on some
> ABI (32-bit with 64-bit time_t), it would require to use the same
> syscall anyway (statx).
>
> I don't really see any gain in adding back this complexity back on
> stat calls, specially with y2038 support requirement.  

But we currently have this code for the x*stat interfaces.  For example,
in sysdeps/unix/sysv/linux/xstat.c:

__xstat (int vers, const char *name, struct stat *buf)
{
  switch (vers)
    {
    case _STAT_VER_KERNEL:
      {
# if STAT_IS_KERNEL_STAT
        /* New kABIs which uses generic pre 64-bit time Linux ABI,
           e.g. csky, nios2  */
        int r = INLINE_SYSCALL_CALL (fstatat64, AT_FDCWD, name, buf, 0);
        return r ?: stat_overflow (buf);
# else
        /* Old kABIs with old non-LFS support, e.g. arm, i386, hppa, m68k,
           microblaze, s390, sh, powerpc, and sparc32.  */
        return INLINE_SYSCALL_CALL (stat, name, buf);
# endif
      }

    default:
      {
# if STAT_IS_KERNEL_STAT
        return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
# else
        struct stat64 buf64;
        int r = INLINE_SYSCALL_CALL (stat64, name, &buf64);
        return r ?: __xstat32_conv (vers, &buf64, buf);
#endif
      }
    }
}

For _STAT_VER_KERNEL, we should be able to call stat directly.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

[Adhemerval Zanella]

On 14/01/2021 11:38, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> On 14/01/2021 09:00, Florian Weimer via Libc-alpha wrote:
>>> Currently, __fxstat may get mapped to the fstat system call, while fstat
>>> goes to fstatat.  This makes sandboxing issues less obvious to debug:
>>>
>>>   <https://bugs.chromium.org/p/chromium/issues/detail?id=1164975>
>>>
>>> Should we change this before the release?  And if yes, in what
>>> direction?
>>
>> Implementing fstat/lstat/stat through fstatat does really simplify
>> the code at *lot* for some architectures. And on others and on some
>> ABI (32-bit with 64-bit time_t), it would require to use the same
>> syscall anyway (statx).
>>
>> I don't really see any gain in adding back this complexity back on
>> stat calls, specially with y2038 support requirement.  
> 
> But we currently have this code for the x*stat interfaces.  For example,
> in sysdeps/unix/sysv/linux/xstat.c:
> 
> __xstat (int vers, const char *name, struct stat *buf)
> {
>   switch (vers)
>     {
>     case _STAT_VER_KERNEL:
>       {
> # if STAT_IS_KERNEL_STAT
>         /* New kABIs which uses generic pre 64-bit time Linux ABI,
>            e.g. csky, nios2  */
>         int r = INLINE_SYSCALL_CALL (fstatat64, AT_FDCWD, name, buf, 0);
>         return r ?: stat_overflow (buf);
> # else
>         /* Old kABIs with old non-LFS support, e.g. arm, i386, hppa, m68k,
>            microblaze, s390, sh, powerpc, and sparc32.  */
>         return INLINE_SYSCALL_CALL (stat, name, buf);
> # endif
>       }
> 
>     default:
>       {
> # if STAT_IS_KERNEL_STAT
>         return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
> # else
>         struct stat64 buf64;
>         int r = INLINE_SYSCALL_CALL (stat64, name, &buf64);
>         return r ?: __xstat32_conv (vers, &buf64, buf);
> #endif
>       }
>     }
> }

For instance for !XSTAT_IS_XSTAT64 (xstat.c) we have that _STAT_VER is
set for _STAT_VER_LINUX on arm and _STAT_VER_KERNEL on csky.

It means that we will need to do something like:

  #if STAT_IS_KERNEL_STAT
  /* csky, nios2  */
  int r = INLINE_SYSCALL_CALL (fstatat64, AT_FDCWD, name, buf, 0);
  return r ?: stat_overflow (buf);
  #else
  /* Old kABIs with old non-LFS support, e.g. arm, i386, hppa, m68k,
     microblaze, s390, sh, powerpc, and sparc32.  */
  struct stat64 buf64;
  int r = INLINE_SYSCALL_CALL (stat64, name, &buf64);
  return r ?: __xstat32_conv (vers, &buf64, buf);
  #endif

And it would have something similar to other implementation. I don't
think this is really an improvement here.

The main issue is this kind of sandboxing implemented by chromium and
other projects are not really future-proof and constrains the libc 
implementation to keep using the old syscalls where the kernel contract
allows to use a more broad one.  In fact, this is *exactly* what kernel
intends to provide a more generic interface instead of multiple ones
to implement the POSIX interfaces.

  

Re: Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

[Florian Weimer]

* Adhemerval Zanella:

> On 14/01/2021 11:38, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> On 14/01/2021 09:00, Florian Weimer via Libc-alpha wrote:
>>>> Currently, __fxstat may get mapped to the fstat system call, while fstat
>>>> goes to fstatat.  This makes sandboxing issues less obvious to debug:
>>>>
>>>>   <https://bugs.chromium.org/p/chromium/issues/detail?id=1164975>
>>>>
>>>> Should we change this before the release?  And if yes, in what
>>>> direction?
>>>
>>> Implementing fstat/lstat/stat through fstatat does really simplify
>>> the code at *lot* for some architectures. And on others and on some
>>> ABI (32-bit with 64-bit time_t), it would require to use the same
>>> syscall anyway (statx).
>>>
>>> I don't really see any gain in adding back this complexity back on
>>> stat calls, specially with y2038 support requirement.  
>> 
>> But we currently have this code for the x*stat interfaces.  For example,
>> in sysdeps/unix/sysv/linux/xstat.c:
>> 
>> __xstat (int vers, const char *name, struct stat *buf)
>> {
>>   switch (vers)
>>     {
>>     case _STAT_VER_KERNEL:
>>       {
>> # if STAT_IS_KERNEL_STAT
>>         /* New kABIs which uses generic pre 64-bit time Linux ABI,
>>            e.g. csky, nios2  */
>>         int r = INLINE_SYSCALL_CALL (fstatat64, AT_FDCWD, name, buf, 0);
>>         return r ?: stat_overflow (buf);
>> # else
>>         /* Old kABIs with old non-LFS support, e.g. arm, i386, hppa, m68k,
>>            microblaze, s390, sh, powerpc, and sparc32.  */
>>         return INLINE_SYSCALL_CALL (stat, name, buf);
>> # endif
>>       }
>> 
>>     default:
>>       {
>> # if STAT_IS_KERNEL_STAT
>>         return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
>> # else
>>         struct stat64 buf64;
>>         int r = INLINE_SYSCALL_CALL (stat64, name, &buf64);
>>         return r ?: __xstat32_conv (vers, &buf64, buf);
>> #endif
>>       }
>>     }
>> }
>
> For instance for !XSTAT_IS_XSTAT64 (xstat.c) we have that _STAT_VER is
> set for _STAT_VER_LINUX on arm and _STAT_VER_KERNEL on csky.

Is there any architecture where we have an fxstat implementation and the
struct stat layout is different from what fstat expects?  To achieve a
consistent system call profile, xfstat for the last version should call
the fstat function, and not use a different inline system call.

> The main issue is this kind of sandboxing implemented by chromium and
> other projects are not really future-proof and constrains the libc
> implementation to keep using the old syscalls where the kernel
> contract allows to use a more broad one.  In fact, this is *exactly*
> what kernel intends to provide a more generic interface instead of
> multiple ones to implement the POSIX interfaces.

Sure, but the problem here is that glibc is not consistent in what
kernel interfaces are used to implement fstat.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

[Adhemerval Zanella]

On 14/01/2021 13:45, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 14/01/2021 11:38, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> On 14/01/2021 09:00, Florian Weimer via Libc-alpha wrote:
>>>>> Currently, __fxstat may get mapped to the fstat system call, while fstat
>>>>> goes to fstatat.  This makes sandboxing issues less obvious to debug:
>>>>>
>>>>>   <https://bugs.chromium.org/p/chromium/issues/detail?id=1164975>
>>>>>
>>>>> Should we change this before the release?  And if yes, in what
>>>>> direction?
>>>>
>>>> Implementing fstat/lstat/stat through fstatat does really simplify
>>>> the code at *lot* for some architectures. And on others and on some
>>>> ABI (32-bit with 64-bit time_t), it would require to use the same
>>>> syscall anyway (statx).
>>>>
>>>> I don't really see any gain in adding back this complexity back on
>>>> stat calls, specially with y2038 support requirement.  
>>>
>>> But we currently have this code for the x*stat interfaces.  For example,
>>> in sysdeps/unix/sysv/linux/xstat.c:
>>>
>>> __xstat (int vers, const char *name, struct stat *buf)
>>> {
>>>   switch (vers)
>>>     {
>>>     case _STAT_VER_KERNEL:
>>>       {
>>> # if STAT_IS_KERNEL_STAT
>>>         /* New kABIs which uses generic pre 64-bit time Linux ABI,
>>>            e.g. csky, nios2  */
>>>         int r = INLINE_SYSCALL_CALL (fstatat64, AT_FDCWD, name, buf, 0);
>>>         return r ?: stat_overflow (buf);
>>> # else
>>>         /* Old kABIs with old non-LFS support, e.g. arm, i386, hppa, m68k,
>>>            microblaze, s390, sh, powerpc, and sparc32.  */
>>>         return INLINE_SYSCALL_CALL (stat, name, buf);
>>> # endif
>>>       }
>>>
>>>     default:
>>>       {
>>> # if STAT_IS_KERNEL_STAT
>>>         return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
>>> # else
>>>         struct stat64 buf64;
>>>         int r = INLINE_SYSCALL_CALL (stat64, name, &buf64);
>>>         return r ?: __xstat32_conv (vers, &buf64, buf);
>>> #endif
>>>       }
>>>     }
>>> }
>>
>> For instance for !XSTAT_IS_XSTAT64 (xstat.c) we have that _STAT_VER is
>> set for _STAT_VER_LINUX on arm and _STAT_VER_KERNEL on csky.
> 
> Is there any architecture where we have an fxstat implementation and the
> struct stat layout is different from what fstat expects?  To achieve a
> consistent system call profile, xfstat for the last version should call
> the fstat function, and not use a different inline system call.

This is a spreadsheet with various supported architectures for the xstat
implementation prior the consolidation and the requirements for each
syscall and/or transformation required [1].

It is quite a mess and that's why I think making only fstatat64 the one
with the syscall logic does simplify a lot.

> 
>> The main issue is this kind of sandboxing implemented by chromium and
>> other projects are not really future-proof and constrains the libc
>> implementation to keep using the old syscalls where the kernel
>> contract allows to use a more broad one.  In fact, this is *exactly*
>> what kernel intends to provide a more generic interface instead of
>> multiple ones to implement the POSIX interfaces.
> 
> Sure, but the problem here is that glibc is not consistent in what
> kernel interfaces are used to implement fstat.

But it does not need to be in fact. This syscall bound semantic keeps
coming as an issue and I agree that it is a potential issue when the
replacement of fallback does not provide the full semantic. But in
this case __NR_newfstatat/__NR_fstatat64/__NR_statx/__NR_fstatat64
is really a superset of all defined stat functions.

> 
> Thanks,
> Florian
> 

[1] https://docs.google.com/spreadsheets/d/e/2PACX-1vRG3-1W8roNgHXVaY8zqIBlGqCRtztdyDUvS_VLLkCnmXANYTmZZvONda6TAKcNSNsqfswMn1_IadE5/pubhtml

Re: Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

[Florian Weimer]

* Adhemerval Zanella:

> This is a spreadsheet with various supported architectures for the xstat
> implementation prior the consolidation and the requirements for each
> syscall and/or transformation required [1].

[1] https://docs.google.com/spreadsheets/d/e/2PACX-1vRG3-1W8roNgHXVaY8zqIBlGqCRtztdyDUvS_VLLkCnmXANYTmZZvONda6TAKcNSNsqfswMn1_IadE5/pubhtml

> It is quite a mess and that's why I think making only fstatat64 the one
> with the syscall logic does simplify a lot.

I still don't get why this wouldn't be a desirable change:

--- a/sysdeps/unix/sysv/linux/fxstat.c
+++ b/sysdeps/unix/sysv/linux/fxstat.c
@@ -35,18 +35,7 @@ __fxstat (int vers, int fd, struct stat *buf)
   switch (vers)
     {
     case _STAT_VER_KERNEL:
-      {
-# if STAT_IS_KERNEL_STAT
-	/* New kABIs which uses generic pre 64-bit time Linux ABI,
-	   e.g. csky, nios2  */
-	int r = INLINE_SYSCALL_CALL (fstat64, fd, buf);
-	return r ?: stat_overflow (buf);
-# else
-	/* Old kABIs with old non-LFS support, e.g. arm, i386, hppa, m68k,
-	   microblaze, s390, sh, powerpc, and sparc.  */
-	return INLINE_SYSCALL_CALL (fstat, fd, buf);
-# endif
-      }
+      return fstat (fd, buf);
 
     default:
       {

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

[Adhemerval Zanella]

On 14/01/2021 14:18, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> This is a spreadsheet with various supported architectures for the xstat
>> implementation prior the consolidation and the requirements for each
>> syscall and/or transformation required [1].
> 
> [1] https://docs.google.com/spreadsheets/d/e/2PACX-1vRG3-1W8roNgHXVaY8zqIBlGqCRtztdyDUvS_VLLkCnmXANYTmZZvONda6TAKcNSNsqfswMn1_IadE5/pubhtml
> 
>> It is quite a mess and that's why I think making only fstatat64 the one
>> with the syscall logic does simplify a lot.
> 
> I still don't get why this wouldn't be a desirable change:
> 
> --- a/sysdeps/unix/sysv/linux/fxstat.c
> +++ b/sysdeps/unix/sysv/linux/fxstat.c
> @@ -35,18 +35,7 @@ __fxstat (int vers, int fd, struct stat *buf)
>    switch (vers)
>      {
>      case _STAT_VER_KERNEL:
> -      {
> -# if STAT_IS_KERNEL_STAT
> -	/* New kABIs which uses generic pre 64-bit time Linux ABI,
> -	   e.g. csky, nios2  */
> -	int r = INLINE_SYSCALL_CALL (fstat64, fd, buf);
> -	return r ?: stat_overflow (buf);
> -# else
> -	/* Old kABIs with old non-LFS support, e.g. arm, i386, hppa, m68k,
> -	   microblaze, s390, sh, powerpc, and sparc.  */
> -	return INLINE_SYSCALL_CALL (fstat, fd, buf);
> -# endif
> -      }
> +      return fstat (fd, buf);
>  
>      default:
>        {

Ah you meant to implement the xstat on top of the fstat. I understood
the other way around, implementing the multiple xstat logic on stat
calls.

I think it should be feasible, I haven't done it because I would like
to touch the compatibility as little as possible to avoid further
breakage.

We will need to handle the various vers though (the comment on xstat
is not really in the correct place, ar, i386, etc sets defaults
version to _STAT_VER_LINUX:

  int
  __xstat (int vers, const char *name, struct stat *buf)
  {
    switch (vers)
      {               
      case _STAT_VER_KERNEL:
        {
  # if STAT_IS_KERNEL_STAT
	  return __fstat (name, buf);     
  # else
          return INLINE_SYSCALL_CALL (stat, name, buf);
  # endif
        }

      /* _STAT_VER_LINUX  */
      default:
        {
  # if STAT_IS_KERNEL_STAT
          return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
  # else
          /* Old kABIs with old non-LFS support, e.g. arm, i386, hppa,
             m68k, microblaze, s390, sh, powerpc, and sparc32.  */
          return __fstat (name, buf);     
  #endif
        }
      }
  }

Re: Should legacy fstat (via __fxstat) and fstat@GLIBC_2.33 call the same syscall?

[Florian Weimer]

* Adhemerval Zanella:

> I think it should be feasible, I haven't done it because I would like
> to touch the compatibility as little as possible to avoid further
> breakage.

Yes, the current model preserves behavior of applications until they are
relinked.  I'm not quite decided yet whether this is a good thing or
not.  In the past, we have done this for behavior changes.  But this
isn't really changing behavior, and the change in syscall profile due to
relinking is quite confusing, too, especially when it comes to browser
sandboxes.

So maybe we should leave this in place for 2.33 and see if we can clean
this up in a future release.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill