[PATCH] Increase judgment on buf.

[PATCH] Increase judgment on buf.

[Peng Fan]

When buf is empty, if it is not checked, the subsequent assignment
operation will trigger a page fault. This is unnecessary.

Signed-off-by: lixing <lixing@loongson.cn>
Signed-off-by: Peng Fan <fanpeng@loongson.cn>
---
 sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
index 3509d3ca6d..b635a8299a 100644
--- a/sysdeps/unix/sysv/linux/fstatat64.c
+++ b/sysdeps/unix/sysv/linux/fstatat64.c
@@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
 {
   /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
      64-bit time_t support is done through statx syscall.  */
-  struct statx tmp;
+  struct statx tmp, *ptr;
+  if (buf)
+	ptr = &tmp;
+  else
+	ptr = NULL;
   int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
-				 STATX_BASIC_STATS, &tmp);
+				 STATX_BASIC_STATS, ptr);
   if (r != 0)
     return r;
 
-- 
2.33.0

Re: [PATCH] Increase judgment on buf.

[Adhemerval Zanella Netto]

On 19/05/23 00:57, Peng Fan wrote:
> When buf is empty, if it is not checked, the subsequent assignment
> operation will trigger a page fault. This is unnecessary.
> 
> Signed-off-by: lixing <lixing@loongson.cn>
> Signed-off-by: Peng Fan <fanpeng@loongson.cn>

The stat family is explicitly marked with nonnull for the input struct
stat buffer, and calling with a NULL argument is an UB.

> ---
>  sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
> index 3509d3ca6d..b635a8299a 100644
> --- a/sysdeps/unix/sysv/linux/fstatat64.c
> +++ b/sysdeps/unix/sysv/linux/fstatat64.c
> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
>  {
>    /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
>       64-bit time_t support is done through statx syscall.  */
> -  struct statx tmp;
> +  struct statx tmp, *ptr;
> +  if (buf)
> +	ptr = &tmp;
> +  else
> +	ptr = NULL;
>    int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
> -				 STATX_BASIC_STATS, &tmp);
> +				 STATX_BASIC_STATS, ptr);
>    if (r != 0)
>      return r;
>  

Re: [PATCH] Increase judgment on buf.

[Carlos O'Donell]

On 5/19/23 07:48, Adhemerval Zanella Netto via Libc-alpha wrote:
> 
> 
> On 19/05/23 00:57, Peng Fan wrote:
>> When buf is empty, if it is not checked, the subsequent assignment
>> operation will trigger a page fault. This is unnecessary.
>>
>> Signed-off-by: lixing <lixing@loongson.cn>
>> Signed-off-by: Peng Fan <fanpeng@loongson.cn>
> 
> The stat family is explicitly marked with nonnull for the input struct
> stat buffer, and calling with a NULL argument is an UB.

Agreed, and "Style and Conventions"
https://sourceware.org/glibc/wiki/Style_and_Conventions
says:
https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program

We should fail catastrophically and early in the case of user bugs.
The segfault generates a core dump at exactly the right point to debug the UB.

>> ---
>>  sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
>> index 3509d3ca6d..b635a8299a 100644
>> --- a/sysdeps/unix/sysv/linux/fstatat64.c
>> +++ b/sysdeps/unix/sysv/linux/fstatat64.c
>> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
>>  {
>>    /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
>>       64-bit time_t support is done through statx syscall.  */
>> -  struct statx tmp;
>> +  struct statx tmp, *ptr;
>> +  if (buf)
>> +	ptr = &tmp;
>> +  else
>> +	ptr = NULL;
>>    int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
>> -				 STATX_BASIC_STATS, &tmp);
>> +				 STATX_BASIC_STATS, ptr);
>>    if (r != 0)
>>      return r;
>>  
> 

-- 
Cheers,
Carlos.

Re: [PATCH] Increase judgment on buf.

[lixing]

在 2023/5/19 下午7:55, Carlos O'Donell via Libc-alpha 写道:
> On 5/19/23 07:48, Adhemerval Zanella Netto via Libc-alpha wrote:
>>
>> On 19/05/23 00:57, Peng Fan wrote:
>>> When buf is empty, if it is not checked, the subsequent assignment
>>> operation will trigger a page fault. This is unnecessary.
>>>
>>> Signed-off-by: lixing <lixing@loongson.cn>
>>> Signed-off-by: Peng Fan <fanpeng@loongson.cn>
>> The stat family is explicitly marked with nonnull for the input struct
>> stat buffer, and calling with a NULL argument is an UB.
> Agreed, and "Style and Conventions"
> https://sourceware.org/glibc/wiki/Style_and_Conventions
> says:
> https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program
>
> We should fail catastrophically and early in the case of user bugs.
> The segfault generates a core dump at exactly the right point to debug the UB.

Yes, LTP fstat03 test pass the buf with NULL. We just want to fail 
earlier during the syscall statx with return value EFAULT if the buf is 
NULL,

but not pass the fault to the struct assignment which trigger SIGSEGV.

>>> ---
>>>   sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
>>> index 3509d3ca6d..b635a8299a 100644
>>> --- a/sysdeps/unix/sysv/linux/fstatat64.c
>>> +++ b/sysdeps/unix/sysv/linux/fstatat64.c
>>> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
>>>   {
>>>     /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32.   Also
>>>        64-bit time_t support is done through statx syscall.  */
>>> -  struct statx tmp;
>>> +  struct statx tmp, *ptr;
>>> +  if (buf)
>>> +	ptr = &tmp;
>>> +  else
>>> +	ptr = NULL;
>>>     int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
>>> -				 STATX_BASIC_STATS, &tmp);
>>> +				 STATX_BASIC_STATS, ptr);
>>>     if (r != 0)
>>>       return r;
>>>   

Re: [PATCH] Increase judgment on buf.

[Xi Ruoyao]

Resend because HTML seems rejected by the list.

Sorry for top posting but my mobile phone is too stupid to format the
reply properly.

Glibc is not "what somebody wants" and it's not a bug nor something we
should change.  In the consensus page we say:
* If you have no contract to accept NULL and you don't immediately
dereference the pointer then use an assert to raise an error when
NULL is passed as an invalid argument. 
* If you have no contract to accept NULL and immediately dereference
the pointer then the segfault is sufficient to indicate the error. 

In this case invoking the syscall is a immediate dereference (well, if
it's not clear we may edit the wiki page to say "including a dereference
in a syscall" or something), and the segfault indicates the error
properly.  Nevertheless, even if the syscall does not segfault,
according to our policy we should deliberately trigger a segfault for
NULL input, not returning EFAULT (unless the standard says we should
return EFAULT).

If a LTP test relies on a EFAULT here, then LTP has a bug.  You should
tell LTP to fix it then.

And adding NULL check here would be a beginning of a sequence of very
very bad moves.  Should we start to add NULL checks everywhere, say
"read", "strlen", "fclose" etc.?  Yes some (stupid) textbook says we
should do this but let's burn such a textbook in fire.  They were
written in 80's or 90's when memory errors were everwhere (and a memory
error was considered "not fatal"), but today memory errors are
considered security vulnerabilities.  Allowing a vulnerable program
continuing to run may make the vulnerability more severe (crashing is a
Deniel of Service, but running with a broken state may lead to Arbitary
Code Execution).

Glibc is a library implementing the ISO C langauage library and POSIX
functions, for supporting real softwares, not for "allowing people who
don't know C or POSIX to program".


-------- 原始邮件 --------
发件人： lixing <lixing@loongson.cn>
日期： 2023年5月20日周六 早上8:29
收件人： Carlos O'Donell <carlos@redhat.com>, Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org>, Peng Fan <fanpeng@loongson.cn>,
libc-alpha@sourceware.org
抄送： Xi Ruoyao <xry111@xry111.site>
主 题： Re: [PATCH] Increase judgment on buf.
> 
> 在 2023/5/19 下午7:55, Carlos O'Donell via Libc-alpha 写道:
> > On 5/19/23 07:48, Adhemerval Zanella Netto via Libc-alpha wrote:
> >>
> >> On 19/05/23 00:57, Peng Fan wrote:
> >>> When buf is empty, if it is not checked, the subsequent assignment
> >>> operation will trigger a page fault. This is unnecessary.
> >>>
> >>> Signed-off-by: lixing <lixing@loongson.cn>
> >>> Signed-off-by: Peng Fan <fanpeng@loongson.cn>
> >> The stat family is explicitly marked with nonnull for the input
> struct
> >> stat buffer, and calling with a NULL argument is an UB.
> > Agreed, and "Style and Conventions"
> > https://sourceware.org/glibc/wiki/Style_and_Conventions
> > says:
> >
>
https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program
> >
> > We should fail catastrophically and early in the case of user bugs.
> > The segfault generates a core dump at exactly the right point to
> debug the UB.
> 
> Yes, LTP fstat03 test pass the buf with NULL. We just want to fail 
> earlier during the syscall statx with return value EFAULT if the buf
> is 
> NULL,
> 
> but not pass the fault to the struct assignment which trigger SIGSEGV.
> 
> >>> ---
> >>>   sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++--
> >>>   1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c
> b/sysdeps/unix/sysv/linux/fstatat64.c
> >>> index 3509d3ca6d..b635a8299a 100644
> >>> --- a/sysdeps/unix/sysv/linux/fstatat64.c
> >>> +++ b/sysdeps/unix/sysv/linux/fstatat64.c
> >>> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char
> *file, struct __stat64_t64 *buf,
> >>>   {
> >>>     /* 32-bit kABI with default 64-bit time_t, e.g. arc,
> riscv32.   Also
> >>>        64-bit time_t support is done through statx syscall.  */
> >>> -  struct statx tmp;
> >>> +  struct statx tmp, *ptr;
> >>> +  if (buf)
> >>> + ptr = &tmp;
> >>> +  else
> >>> + ptr = NULL;
> >>>     int r = INTERNAL_SYSCALL_CALL (statx, fd, file,
> AT_NO_AUTOMOUNT | flag,
> >>> - STATX_BASIC_STATS, &tmp);
> >>> + STATX_BASIC_STATS, ptr);
> >>>     if (r != 0)
> >>>       return r;
> >>>   
> 

-- 
Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and
Technology, Xidian University

Re: [PATCH] Increase judgment on buf.

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On 2023-05-20 03:19, Xi Ruoyao via Libc-alpha wrote:
> If a LTP test relies on a EFAULT here, then LTP has a bug.  You should
> tell LTP to fix it then.

LTP fstat03 allows either EFAULT or SIGSEGV, but no other behavior, when 
'stat' is passed a null pointer.

Therefore that test is too strict for POSIX, as the POSIX behavior is 
undefined.

It's also too strict for GNU/Linux, because it insists on SIGSEGV 
whereas any terminating signal will do (e.g., SIGABRT).

Proposed LTP patch attached.

[-- Attachment #2: 0001-fstat03-wrongly-required-SIGSEGV.patch --]
[-- Type: text/x-patch, Size: 1490 bytes --]

From 8a2b3f75d2b61e5e4ec0a2c93f3db62584daf1cd Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 21 May 2023 01:15:40 -0700
Subject: [PATCH] fstat03 wrongly required SIGSEGV

Change fstat03 to allow the syscall to signal SIGABRT (or any other
signal) instead of SIGSEGV when a null pointer is passed, as POSIX
says behavior is undefined and the glibc behavior (which is a bit
better defined) could be any signal, not necessarily SIGSEGV.

Signed-off-by: Paul Eggert <eggert@cs.ucla.edu>
---
 testcases/kernel/syscalls/fstat/fstat03.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/fstat/fstat03.c b/testcases/kernel/syscalls/fstat/fstat03.c
index 4ff37e882..a400692e9 100644
--- a/testcases/kernel/syscalls/fstat/fstat03.c
+++ b/testcases/kernel/syscalls/fstat/fstat03.c
@@ -10,7 +10,7 @@
  * 1) Calls fstat() with closed file descriptor
  *    -> EBADF
  * 2) Calls fstat() with an invalid address for stat structure
- *    -> EFAULT (or receive signal SIGSEGV)
+ *    -> EFAULT (or receive signal)
  */
 
 #include <errno.h>
@@ -70,8 +70,8 @@ static void run(unsigned int tc_num)
 	}
 	SAFE_WAITPID(pid, &status, 0);
 
-	if (tcases[tc_num].exp_err == EFAULT && WTERMSIG(status) == SIGSEGV) {
-		tst_res(TPASS, "fstat() failed as expected with SIGSEGV");
+	if (tcases[tc_num].exp_err == EFAULT && WIFSIGNALED(status)) {
+		tst_res(TPASS, "fstat() failed as expected with a signal");
 		return;
 	}
 
-- 
2.39.2

Re: [PATCH] Increase judgment on buf.

[Andreas Schwab]

On Mai 21 2023, Paul Eggert wrote:

> +	if (tcases[tc_num].exp_err == EFAULT && WIFSIGNALED(status)) {

Can this condition ever be true?

-- 
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] Increase judgment on buf.

[Paul Eggert]

On 2023-05-21 01:27, Andreas Schwab wrote:
>> +	if (tcases[tc_num].exp_err == EFAULT && WIFSIGNALED(status)) {
> Can this condition ever be true?

I don't see why not, if fstat(fd_ok, 0) does a SIGABRT or similar. But 
perhaps we're interpreting "Can" differently.

Re: [PATCH] Increase judgment on buf.

[Andreas Schwab]

I have misinterpreted the first part of the condition.

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