[PATCH] linux: Use stat_overflow to check overflow in fstatat

[PATCH] linux: Use stat_overflow to check overflow in fstatat

[Stafford Horne]

After a recent rebase of the OpenRISC port I am working on the build
started to fail with:

    ../sysdeps/unix/sysv/linux/fstatat.c: In function '__fstatat':
    ../sysdeps/unix/sysv/linux/fstatat.c:35:21: error: 'struct stat' has no member named '__st_ino_pad'
       35 |   if (r == 0 && (buf->__st_ino_pad != 0
	  |                     ^~
    ../sysdeps/unix/sysv/linux/fstatat.c:36:24: error: 'struct stat' has no member named '__st_size_pad'
       36 |                  || buf->__st_size_pad != 0
	  |                        ^~
    ../sysdeps/unix/sysv/linux/fstatat.c:37:24: error: 'struct stat' has no member named '__st_blocks_pad'
       37 |                  || buf->__st_blocks_pad != 0))
	  |                        ^~

This seems to be caused by 6073bae64c ("linux: Disentangle fstatat from
fxstatat").  Which introduced the checks for overflow using the
stat->__st_*_pad fields.  These do not exist on all architectures, I
cannot even find any that do exist by grepping code.

The fix is to use the stat_overflow function which only runs the
overflow checks if the pad fields are available.

Note, This is not xstat but I use the xstatover.h header.  I hope that
is not an issue.

Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
 sysdeps/unix/sysv/linux/fstatat.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fstatat.c b/sysdeps/unix/sysv/linux/fstatat.c
index 59efff615f..5bff931183 100644
--- a/sysdeps/unix/sysv/linux/fstatat.c
+++ b/sysdeps/unix/sysv/linux/fstatat.c
@@ -22,6 +22,7 @@
 
 #if !XSTAT_IS_XSTAT64
 # include <kstat_cp.h>
+# include <xstatover.h>
 
 int
 __fstatat (int fd, const char *file, struct stat *buf, int flag)
@@ -32,10 +33,7 @@ __fstatat (int fd, const char *file, struct stat *buf, int flag)
   /* New kABIs which uses generic pre 64-bit time Linux ABI, e.g.
      csky, nios2  */
   r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
-  if (r == 0 && (buf->__st_ino_pad != 0
-		 || buf->__st_size_pad != 0
-		 || buf->__st_blocks_pad != 0))
-    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOVERFLOW);
+  return r ?: stat_overflow (buf);
 # else
 #  ifdef __NR_fstatat64
   /* Old KABIs with old non-LFS support, e.g. arm, i386, hppa, m68k, mips32,
-- 
2.26.2

Re: [PATCH] linux: Use stat_overflow to check overflow in fstatat

[Andreas Schwab]

On Jan 14 2021, Stafford Horne via Libc-alpha wrote:

> This seems to be caused by 6073bae64c ("linux: Disentangle fstatat from
> fxstatat").  Which introduced the checks for overflow using the
> stat->__st_*_pad fields.  These do not exist on all architectures, I
> cannot even find any that do exist by grepping code.

See __field64 in sysdeps/unix/sysv/linux/generic/bits/struct_stat.h.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] linux: Use stat_overflow to check overflow in fstatat

[Stafford Horne]

On Thu, Jan 14, 2021 at 10:29:17AM +0100, Andreas Schwab wrote:
> On Jan 14 2021, Stafford Horne via Libc-alpha wrote:
> 
> > This seems to be caused by 6073bae64c ("linux: Disentangle fstatat from
> > fxstatat").  Which introduced the checks for overflow using the
> > stat->__st_*_pad fields.  These do not exist on all architectures, I
> > cannot even find any that do exist by grepping code.
> 
> See __field64 in sysdeps/unix/sysv/linux/generic/bits/struct_stat.h.

Thank's I was only finding kernel_stat.h, this makes sense now.

I think the patch is still valid, though.

-Stafford

Re: [PATCH] linux: Use stat_overflow to check overflow in fstatat

[Stafford Horne]

Ping Adhemerval on this one.

I still thing the below is needed for the STAT_IS_KERNEL_STAT
cases.

On Thu, Jan 14, 2021 at 05:13:49PM +0900, Stafford Horne wrote:
> After a recent rebase of the OpenRISC port I am working on the build
> started to fail with:
> 
>     ../sysdeps/unix/sysv/linux/fstatat.c: In function '__fstatat':
>     ../sysdeps/unix/sysv/linux/fstatat.c:35:21: error: 'struct stat' has no member named '__st_ino_pad'
>        35 |   if (r == 0 && (buf->__st_ino_pad != 0
> 	  |                     ^~
>     ../sysdeps/unix/sysv/linux/fstatat.c:36:24: error: 'struct stat' has no member named '__st_size_pad'
>        36 |                  || buf->__st_size_pad != 0
> 	  |                        ^~
>     ../sysdeps/unix/sysv/linux/fstatat.c:37:24: error: 'struct stat' has no member named '__st_blocks_pad'
>        37 |                  || buf->__st_blocks_pad != 0))
> 	  |                        ^~
> 
> This seems to be caused by 6073bae64c ("linux: Disentangle fstatat from
> fxstatat").  Which introduced the checks for overflow using the
> stat->__st_*_pad fields.  These do not exist on all architectures, I
> cannot even find any that do exist by grepping code.
> 
> The fix is to use the stat_overflow function which only runs the
> overflow checks if the pad fields are available.
> 
> Note, This is not xstat but I use the xstatover.h header.  I hope that
> is not an issue.
> 
> Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
>  sysdeps/unix/sysv/linux/fstatat.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/fstatat.c b/sysdeps/unix/sysv/linux/fstatat.c
> index 59efff615f..5bff931183 100644
> --- a/sysdeps/unix/sysv/linux/fstatat.c
> +++ b/sysdeps/unix/sysv/linux/fstatat.c
> @@ -22,6 +22,7 @@
>  
>  #if !XSTAT_IS_XSTAT64
>  # include <kstat_cp.h>
> +# include <xstatover.h>
>  
>  int
>  __fstatat (int fd, const char *file, struct stat *buf, int flag)
> @@ -32,10 +33,7 @@ __fstatat (int fd, const char *file, struct stat *buf, int flag)
>    /* New kABIs which uses generic pre 64-bit time Linux ABI, e.g.
>       csky, nios2  */
>    r = INTERNAL_SYSCALL_CALL (fstatat64, fd, file, buf, flag);
> -  if (r == 0 && (buf->__st_ino_pad != 0
> -		 || buf->__st_size_pad != 0
> -		 || buf->__st_blocks_pad != 0))
> -    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOVERFLOW);
> +  return r ?: stat_overflow (buf);
>  # else
>  #  ifdef __NR_fstatat64
>    /* Old KABIs with old non-LFS support, e.g. arm, i386, hppa, m68k, mips32,
> -- 
> 2.26.2
> 

Re: [PATCH] linux: Use stat_overflow to check overflow in fstatat

[Adhemerval Zanella]

On 27/01/2021 09:40, Stafford Horne wrote:
> Ping Adhemerval on this one.
> 
> I still thing the below is needed for the STAT_IS_KERNEL_STAT
> cases.
> 

Why OpenRISC is not setting XSTAT_IS_XSTAT64 ? As a possible newer
ABI, I see no pointing in support old non-LFS ABI.

Re: [PATCH] linux: Use stat_overflow to check overflow in fstatat

[Stafford Horne]

On Wed, Jan 27, 2021 at 10:28:58AM -0300, Adhemerval Zanella wrote:
> 
> 
> On 27/01/2021 09:40, Stafford Horne wrote:
> > Ping Adhemerval on this one.
> > 
> > I still thing the below is needed for the STAT_IS_KERNEL_STAT
> > cases.
> > 
> 
> Why OpenRISC is not setting XSTAT_IS_XSTAT64 ? As a possible newer
> ABI, I see no pointing in support old non-LFS ABI.

OK, I got it.
I'll try to turn that on.  I was not so clear on what all of the *STAT_IS_*
macros defined, so I was trying to just go with the generic defaults.

On OpenRISC which is only 32-bit XSTAT_IS_XSTAT64 is set to zero.

As per: sysdeps/unix/sysv/linux/generic/kernel_stat.h

    #if __WORDSIZE == 64
    # define XSTAT_IS_XSTAT64 1
    #else
    # define XSTAT_IS_XSTAT64 0
    #endif

Again, I'll see what I can do.

-Stafford

Re: [PATCH] linux: Use stat_overflow to check overflow in fstatat

[Stafford Horne]

On Wed, Jan 27, 2021 at 10:28:58AM -0300, Adhemerval Zanella wrote:
> 
> 
> On 27/01/2021 09:40, Stafford Horne wrote:
> > Ping Adhemerval on this one.
> > 
> > I still thing the below is needed for the STAT_IS_KERNEL_STAT
> > cases.
> > 
> 
> Why OpenRISC is not setting XSTAT_IS_XSTAT64 ? As a possible newer
> ABI, I see no pointing in support old non-LFS ABI.

I got it all working without this patch after:

  - Setting XSTAT_IS_XSTAT64 and a few other kernel_stats.h
  - Updating my shlib-version to 2.33

Note to myself in glibc LFS stands for Large File System. I see that in many
parts of the stats code but couldn't find the actual acronym explained, I found
it in the manual, it was hard to search for on the internet.

Thanks.

-Stafford

Re: [PATCH] linux: Use stat_overflow to check overflow in fstatat

[Adhemerval Zanella]

On 29/01/2021 19:25, Stafford Horne wrote:
> On Wed, Jan 27, 2021 at 10:28:58AM -0300, Adhemerval Zanella wrote:
>>
>>
>> On 27/01/2021 09:40, Stafford Horne wrote:
>>> Ping Adhemerval on this one.
>>>
>>> I still thing the below is needed for the STAT_IS_KERNEL_STAT
>>> cases.
>>>
>>
>> Why OpenRISC is not setting XSTAT_IS_XSTAT64 ? As a possible newer
>> ABI, I see no pointing in support old non-LFS ABI.
> 
> I got it all working without this patch after:
> 
>   - Setting XSTAT_IS_XSTAT64 and a few other kernel_stats.h
>   - Updating my shlib-version to 2.33
> 
> Note to myself in glibc LFS stands for Large File System. I see that in many
> parts of the stats code but couldn't find the actual acronym explained, I found
> it in the manual, it was hard to search for on the internet.
> 
> Thanks.
> 
> -Stafford
> 

I think I will need to revise the internal stat defaults, the whole 
idea of my stat consolidation is to avoid this very of issue for 
newer ports.

Ideally newer ports should not have to support non-LFS calls, which
means that sysdeps/unix/sysv/linux/[f,l]stat[at].c should be empty
TU.

However it seems that the default XSTAT_IS_XSTAT64 is still 0 on
both the Linux and generic one (the generic defines it to 1 for
__WORDSIZE == 64 at least).

For 2.34 I will consolidate the kernel_stat.h, so the default values
will be:

  #define STAT_IS_KERNEL_STAT 1
  #define XSTAT_IS_XSTAT64    1
  #define STATFS_IS_STATFS64  1

The STAT_IS_KERNEL_STAT is only used for non-LFS call and old xstat
support so it wouldn't matter for newer ABIs.  The only important flag
here is XSTAT_IS_XSTAT64 (which is currently misleading since xstat
is the compat symbols now).

It would probably need to consolidate the statfs code, which is another
messy one.