mbox series

[0/3] safe_macros: Fix undefined behaviour in vararg handling

Message ID 20221123144746.590890-1-tudor.cretu@arm.com
Headers show
Series safe_macros: Fix undefined behaviour in vararg handling | expand

Message

Tudor Cretu Nov. 23, 2022, 2:47 p.m. UTC
Accessing elements in an empty va_list results in undefined behaviour[0]
that can include accessing arbitrary stack memory. While typically this
doesn't raise a fault, some new more security-oriented architectures
(e.g. CHERI[1] or Morello[2]) don't allow it.

Therefore, remove the variadicness from safe_* wrappers that always call
the functions with the optional argument included.

Adapt the respective SAFE_* macros to handle the change by passing a
default argument if they're omitted.

[0]: [ISO/IEC 9899:2011] Programming Languages—C, 3rd ed, paragraph 7.16.1.1
[1]: https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/
[2]: https://www.morello-project.org/

Tudor Cretu (3):
  safe_open: Fix undefined behaviour in vararg handling
  safe_openat: Fix undefined behaviour in vararg handling
  safe_semctl: Fix undefined behaviour in vararg handling

 include/old/safe_macros.h   |  6 ++++--
 include/safe_macros_fn.h    |  3 ++-
 include/tst_safe_file_at.h  | 10 ++++++----
 include/tst_safe_macros.h   |  6 ++++--
 include/tst_safe_sysv_ipc.h | 14 +++++++++-----
 lib/safe_macros.c           | 13 +------------
 lib/tst_cgroup.c            |  2 +-
 lib/tst_safe_file_at.c      | 11 +++--------
 lib/tst_safe_sysv_ipc.c     | 10 +---------
 9 files changed, 31 insertions(+), 44 deletions(-)

Comments

Petr Vorel Nov. 29, 2022, 11:01 a.m. UTC | #1
Hi Tudor,

> Accessing elements in an empty va_list results in undefined behaviour[0]
> that can include accessing arbitrary stack memory. While typically this
> doesn't raise a fault, some new more security-oriented architectures
> (e.g. CHERI[1] or Morello[2]) don't allow it.

> Therefore, remove the variadicness from safe_* wrappers that always call
> the functions with the optional argument included.

> Adapt the respective SAFE_* macros to handle the change by passing a
> default argument if they're omitted.

Thanks for an interesting patchset!

I wonder if it's a correct approach as C API allows
int open(const char *pathname, int flags);
we will replace it 
int open(const char *pathname, int flags, mode_t mode);
where mode is 0.
But as it's only in safe_* it should be ok.
We still have some open() tests without mode, i.e.
testcases/kernel/syscalls/open/open06.c

Unfortunately some tests need to be adjusted, at least all tests in
testcases/kernel/syscalls/fgetxattr will fail due int-conversion,
as they use NULL as the third parameter:

$ export CFLAGS="-Werror=conversion"
$ ./configure
$ cd testcases/kernel/syscalls/fgetxattr
$ make clean
$ make V=1
gcc -I../../../../include -I../../../../include -I../../../../include/old/ -Werror=int-conversion -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -std=gnu99 -L../../../../lib fgetxattr01.c  -lltp -o fgetxattr01
In file included from ../../../../include/tst_test.h:98,
                 from fgetxattr01.c:34:
fgetxattr01.c: In function ‘setup’:
../../../../include/tst_safe_macros.h:90:67: error: passing argument 6 of ‘safe_open’ makes integer from pointer without a cast [-Werror=int-conversion]
   90 |         safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), (mode))
      |                                                                   ^~~~~~
      |                                                                   |
      |                                                                   void *
../../../../include/tst_safe_macros.h:93:9: note: in expansion of macro ‘__SAFE_OPEN’
   93 |         __SAFE_OPEN((pathname), (oflags), ##__VA_ARGS__, 0)
      |         ^~~~~~~~~~~
fgetxattr01.c:114:14: note: in expansion of macro ‘SAFE_OPEN’
  114 |         fd = SAFE_OPEN(FNAME, O_RDONLY, NULL);
      |              ^~~~~~~~~
In file included from ../../../../include/tst_safe_macros.h:24:
../../../../include/safe_macros_fn.h:78:22: note: expected ‘mode_t’ {aka ‘unsigned int’} but argument is of type ‘void *’
   78 |               mode_t mode);
      |               ~~~~~~~^~~~
cc1: some warnings being treated as errors
make: *** [../../../../include/mk/rules.mk:43: fgetxattr01] Error 1

Found on recent openSUSE Tumbleweed which has 2.36. Also our CI has caught it on
Fedora (which also uses 2.36):
https://github.com/pevik/ltp/actions/runs/3573215390/jobs/6007016572

Kind regards,
Petr

> [0]: [ISO/IEC 9899:2011] Programming Languages—C, 3rd ed, paragraph 7.16.1.1
> [1]: https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/
> [2]: https://www.morello-project.org/

> Tudor Cretu (3):
>   safe_open: Fix undefined behaviour in vararg handling
>   safe_openat: Fix undefined behaviour in vararg handling
>   safe_semctl: Fix undefined behaviour in vararg handling

>  include/old/safe_macros.h   |  6 ++++--
>  include/safe_macros_fn.h    |  3 ++-
>  include/tst_safe_file_at.h  | 10 ++++++----
>  include/tst_safe_macros.h   |  6 ++++--
>  include/tst_safe_sysv_ipc.h | 14 +++++++++-----
>  lib/safe_macros.c           | 13 +------------
>  lib/tst_cgroup.c            |  2 +-
>  lib/tst_safe_file_at.c      | 11 +++--------
>  lib/tst_safe_sysv_ipc.c     | 10 +---------
>  9 files changed, 31 insertions(+), 44 deletions(-)
Petr Vorel Nov. 29, 2022, 11:06 a.m. UTC | #2
> Hi Tudor,

> > Accessing elements in an empty va_list results in undefined behaviour[0]
> > that can include accessing arbitrary stack memory. While typically this
> > doesn't raise a fault, some new more security-oriented architectures
> > (e.g. CHERI[1] or Morello[2]) don't allow it.

> > Therefore, remove the variadicness from safe_* wrappers that always call
> > the functions with the optional argument included.

> > Adapt the respective SAFE_* macros to handle the change by passing a
> > default argument if they're omitted.

> Thanks for an interesting patchset!

> I wonder if it's a correct approach as C API allows
> int open(const char *pathname, int flags);
> we will replace it 
> int open(const char *pathname, int flags, mode_t mode);
> where mode is 0.
> But as it's only in safe_* it should be ok.
> We still have some open() tests without mode, i.e.
> testcases/kernel/syscalls/open/open06.c

> Unfortunately some tests need to be adjusted, at least all tests in
> testcases/kernel/syscalls/fgetxattr will fail due int-conversion,
> as they use NULL as the third parameter:

> $ export CFLAGS="-Werror=conversion"
> $ ./configure
> $ cd testcases/kernel/syscalls/fgetxattr
> $ make clean
> $ make V=1
> gcc -I../../../../include -I../../../../include -I../../../../include/old/ -Werror=int-conversion -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -std=gnu99 -L../../../../lib fgetxattr01.c  -lltp -o fgetxattr01
> In file included from ../../../../include/tst_test.h:98,
>                  from fgetxattr01.c:34:
> fgetxattr01.c: In function ‘setup’:
> ../../../../include/tst_safe_macros.h:90:67: error: passing argument 6 of ‘safe_open’ makes integer from pointer without a cast [-Werror=int-conversion]
>    90 |         safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), (mode))
>       |                                                                   ^~~~~~

>       |                                                                   void *
> ../../../../include/tst_safe_macros.h:93:9: note: in expansion of macro ‘__SAFE_OPEN’
>    93 |         __SAFE_OPEN((pathname), (oflags), ##__VA_ARGS__, 0)
>       |         ^~~~~~~~~~~
> fgetxattr01.c:114:14: note: in expansion of macro ‘SAFE_OPEN’
>   114 |         fd = SAFE_OPEN(FNAME, O_RDONLY, NULL);
>       |              ^~~~~~~~~
> In file included from ../../../../include/tst_safe_macros.h:24:
> ../../../../include/safe_macros_fn.h:78:22: note: expected ‘mode_t’ {aka ‘unsigned int’} but argument is of type ‘void *’
>    78 |               mode_t mode);
>       |               ~~~~~~~^~~~
> cc1: some warnings being treated as errors
> make: *** [../../../../include/mk/rules.mk:43: fgetxattr01] Error 1

NOTE: this is from gcc, but obviously also clang has the same problem,
even without -Werror=int-conversion in CFLAGS, obviously it's the default.
That's why it was found on Fedora, which we test with clang.

Kind regards,
Petr

> Found on recent openSUSE Tumbleweed which has 2.36. Also our CI has caught it on
> Fedora (which also uses 2.36):
> https://github.com/pevik/ltp/actions/runs/3573215390/jobs/6007016572

> Kind regards,
> Petr

> > [0]: [ISO/IEC 9899:2011] Programming Languages—C, 3rd ed, paragraph 7.16.1.1
> > [1]: https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/
> > [2]: https://www.morello-project.org/

> > Tudor Cretu (3):
> >   safe_open: Fix undefined behaviour in vararg handling
> >   safe_openat: Fix undefined behaviour in vararg handling
> >   safe_semctl: Fix undefined behaviour in vararg handling

> >  include/old/safe_macros.h   |  6 ++++--
> >  include/safe_macros_fn.h    |  3 ++-
> >  include/tst_safe_file_at.h  | 10 ++++++----
> >  include/tst_safe_macros.h   |  6 ++++--
> >  include/tst_safe_sysv_ipc.h | 14 +++++++++-----
> >  lib/safe_macros.c           | 13 +------------
> >  lib/tst_cgroup.c            |  2 +-
> >  lib/tst_safe_file_at.c      | 11 +++--------
> >  lib/tst_safe_sysv_ipc.c     | 10 +---------
> >  9 files changed, 31 insertions(+), 44 deletions(-)
Richard Palethorpe Nov. 29, 2022, 11:11 a.m. UTC | #3
Hello,

Petr Vorel <pvorel@suse.cz> writes:

>> Hi Tudor,
>
>> > Accessing elements in an empty va_list results in undefined behaviour[0]
>> > that can include accessing arbitrary stack memory. While typically this
>> > doesn't raise a fault, some new more security-oriented architectures
>> > (e.g. CHERI[1] or Morello[2]) don't allow it.
>
>> > Therefore, remove the variadicness from safe_* wrappers that always call
>> > the functions with the optional argument included.
>
>> > Adapt the respective SAFE_* macros to handle the change by passing a
>> > default argument if they're omitted.
>
>> Thanks for an interesting patchset!
>
>> I wonder if it's a correct approach as C API allows

Perhaps, muslc does the following for open()

	if ((flags & O_CREAT) || (flags & O_TMPFILE) == O_TMPFILE) {
		va_list ap;
		va_start(ap, flags);
		mode = va_arg(ap, mode_t);
		va_end(ap);
	}

So it's only accessed if we need the mode. If the error below can be
fixed with the current approach I'd also be happy.


>> int open(const char *pathname, int flags);
>> we will replace it 
>> int open(const char *pathname, int flags, mode_t mode);
>> where mode is 0.
>> But as it's only in safe_* it should be ok.
>> We still have some open() tests without mode, i.e.
>> testcases/kernel/syscalls/open/open06.c
>
>> Unfortunately some tests need to be adjusted, at least all tests in
>> testcases/kernel/syscalls/fgetxattr will fail due int-conversion,
>> as they use NULL as the third parameter:
>
>> $ export CFLAGS="-Werror=conversion"
>> $ ./configure
>> $ cd testcases/kernel/syscalls/fgetxattr
>> $ make clean
>> $ make V=1
>> gcc -I../../../../include -I../../../../include -I../../../../include/old/ -Werror=int-conversion -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -std=gnu99 -L../../../../lib fgetxattr01.c  -lltp -o fgetxattr01
>> In file included from ../../../../include/tst_test.h:98,
>>                  from fgetxattr01.c:34:
>> fgetxattr01.c: In function ‘setup’:
>> ../../../../include/tst_safe_macros.h:90:67: error: passing argument 6 of ‘safe_open’ makes integer from pointer without a cast [-Werror=int-conversion]
>>    90 |         safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), (mode))
>>       |                                                                   ^~~~~~
>
>>       |                                                                   void *
>> ../../../../include/tst_safe_macros.h:93:9: note: in expansion of macro ‘__SAFE_OPEN’
>>    93 |         __SAFE_OPEN((pathname), (oflags), ##__VA_ARGS__, 0)
>>       |         ^~~~~~~~~~~
>> fgetxattr01.c:114:14: note: in expansion of macro ‘SAFE_OPEN’
>>   114 |         fd = SAFE_OPEN(FNAME, O_RDONLY, NULL);
>>       |              ^~~~~~~~~
>> In file included from ../../../../include/tst_safe_macros.h:24:
>> ../../../../include/safe_macros_fn.h:78:22: note: expected ‘mode_t’ {aka ‘unsigned int’} but argument is of type ‘void *’
>>    78 |               mode_t mode);
>>       |               ~~~~~~~^~~~
>> cc1: some warnings being treated as errors
>> make: *** [../../../../include/mk/rules.mk:43: fgetxattr01] Error 1
>
> NOTE: this is from gcc, but obviously also clang has the same problem,
> even without -Werror=int-conversion in CFLAGS, obviously it's the default.
> That's why it was found on Fedora, which we test with clang.
>
> Kind regards,
> Petr
>
>> Found on recent openSUSE Tumbleweed which has 2.36. Also our CI has caught it on
>> Fedora (which also uses 2.36):
>> https://github.com/pevik/ltp/actions/runs/3573215390/jobs/6007016572
>
>> Kind regards,
>> Petr
>
>> > [0]: [ISO/IEC 9899:2011] Programming Languages—C, 3rd ed, paragraph 7.16.1.1
>> > [1]: https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/
>> > [2]: https://www.morello-project.org/
>
>> > Tudor Cretu (3):
>> >   safe_open: Fix undefined behaviour in vararg handling
>> >   safe_openat: Fix undefined behaviour in vararg handling
>> >   safe_semctl: Fix undefined behaviour in vararg handling
>
>> >  include/old/safe_macros.h   |  6 ++++--
>> >  include/safe_macros_fn.h    |  3 ++-
>> >  include/tst_safe_file_at.h  | 10 ++++++----
>> >  include/tst_safe_macros.h   |  6 ++++--
>> >  include/tst_safe_sysv_ipc.h | 14 +++++++++-----
>> >  lib/safe_macros.c           | 13 +------------
>> >  lib/tst_cgroup.c            |  2 +-
>> >  lib/tst_safe_file_at.c      | 11 +++--------
>> >  lib/tst_safe_sysv_ipc.c     | 10 +---------
>> >  9 files changed, 31 insertions(+), 44 deletions(-)
Richard Palethorpe Nov. 29, 2022, 11:27 a.m. UTC | #4
Hello,

This looks pretty trivial to fix actually. We shouldn't pass NULL as
mode. If it works I'll add the fix and push.

Richard Palethorpe <rpalethorpe@suse.de> writes:

> Hello,
>
> Petr Vorel <pvorel@suse.cz> writes:
>
>>> Hi Tudor,
>>
>>> > Accessing elements in an empty va_list results in undefined behaviour[0]
>>> > that can include accessing arbitrary stack memory. While typically this
>>> > doesn't raise a fault, some new more security-oriented architectures
>>> > (e.g. CHERI[1] or Morello[2]) don't allow it.
>>
>>> > Therefore, remove the variadicness from safe_* wrappers that always call
>>> > the functions with the optional argument included.
>>
>>> > Adapt the respective SAFE_* macros to handle the change by passing a
>>> > default argument if they're omitted.
>>
>>> Thanks for an interesting patchset!
>>
>>> I wonder if it's a correct approach as C API allows
>
> Perhaps, muslc does the following for open()
>
> 	if ((flags & O_CREAT) || (flags & O_TMPFILE) == O_TMPFILE) {
> 		va_list ap;
> 		va_start(ap, flags);
> 		mode = va_arg(ap, mode_t);
> 		va_end(ap);
> 	}
>
> So it's only accessed if we need the mode. If the error below can be
> fixed with the current approach I'd also be happy.
>
>
>>> int open(const char *pathname, int flags);
>>> we will replace it 
>>> int open(const char *pathname, int flags, mode_t mode);
>>> where mode is 0.
>>> But as it's only in safe_* it should be ok.
>>> We still have some open() tests without mode, i.e.
>>> testcases/kernel/syscalls/open/open06.c
>>
>>> Unfortunately some tests need to be adjusted, at least all tests in
>>> testcases/kernel/syscalls/fgetxattr will fail due int-conversion,
>>> as they use NULL as the third parameter:
>>
>>> $ export CFLAGS="-Werror=conversion"
>>> $ ./configure
>>> $ cd testcases/kernel/syscalls/fgetxattr
>>> $ make clean
>>> $ make V=1
>>> gcc -I../../../../include -I../../../../include -I../../../../include/old/ -Werror=int-conversion -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -std=gnu99 -L../../../../lib fgetxattr01.c  -lltp -o fgetxattr01
>>> In file included from ../../../../include/tst_test.h:98,
>>>                  from fgetxattr01.c:34:
>>> fgetxattr01.c: In function ‘setup’:
>>> ../../../../include/tst_safe_macros.h:90:67: error: passing argument 6 of ‘safe_open’ makes integer from pointer without a cast [-Werror=int-conversion]
>>>    90 |         safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), (mode))
>>>       |                                                                   ^~~~~~
>>
>>>       |                                                                   void *
>>> ../../../../include/tst_safe_macros.h:93:9: note: in expansion of macro ‘__SAFE_OPEN’
>>>    93 |         __SAFE_OPEN((pathname), (oflags), ##__VA_ARGS__, 0)
>>>       |         ^~~~~~~~~~~
>>> fgetxattr01.c:114:14: note: in expansion of macro ‘SAFE_OPEN’
>>>   114 |         fd = SAFE_OPEN(FNAME, O_RDONLY, NULL);
>>>       |              ^~~~~~~~~
>>> In file included from ../../../../include/tst_safe_macros.h:24:
>>> ../../../../include/safe_macros_fn.h:78:22: note: expected ‘mode_t’ {aka ‘unsigned int’} but argument is of type ‘void *’
>>>    78 |               mode_t mode);
>>>       |               ~~~~~~~^~~~
>>> cc1: some warnings being treated as errors
>>> make: *** [../../../../include/mk/rules.mk:43: fgetxattr01] Error 1
>>
>> NOTE: this is from gcc, but obviously also clang has the same problem,
>> even without -Werror=int-conversion in CFLAGS, obviously it's the default.
>> That's why it was found on Fedora, which we test with clang.
>>
>> Kind regards,
>> Petr
>>
>>> Found on recent openSUSE Tumbleweed which has 2.36. Also our CI has caught it on
>>> Fedora (which also uses 2.36):
>>> https://github.com/pevik/ltp/actions/runs/3573215390/jobs/6007016572
>>
>>> Kind regards,
>>> Petr
>>
>>> > [0]: [ISO/IEC 9899:2011] Programming Languages—C, 3rd ed, paragraph 7.16.1.1
>>> > [1]: https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/
>>> > [2]: https://www.morello-project.org/
>>
>>> > Tudor Cretu (3):
>>> >   safe_open: Fix undefined behaviour in vararg handling
>>> >   safe_openat: Fix undefined behaviour in vararg handling
>>> >   safe_semctl: Fix undefined behaviour in vararg handling
>>
>>> >  include/old/safe_macros.h   |  6 ++++--
>>> >  include/safe_macros_fn.h    |  3 ++-
>>> >  include/tst_safe_file_at.h  | 10 ++++++----
>>> >  include/tst_safe_macros.h   |  6 ++++--
>>> >  include/tst_safe_sysv_ipc.h | 14 +++++++++-----
>>> >  lib/safe_macros.c           | 13 +------------
>>> >  lib/tst_cgroup.c            |  2 +-
>>> >  lib/tst_safe_file_at.c      | 11 +++--------
>>> >  lib/tst_safe_sysv_ipc.c     | 10 +---------
>>> >  9 files changed, 31 insertions(+), 44 deletions(-)
Petr Vorel Nov. 29, 2022, 11:32 a.m. UTC | #5
> Hello,

> This looks pretty trivial to fix actually. We shouldn't pass NULL as
> mode. If it works I'll add the fix and push.
Yes, it fixes it, good point. I was also surprised by NULL.

to whole patchset:
Reviewed-by: Petr Vorel <pvorel@suse.cz>


Kind regards,
Petr
Tudor Cretu Nov. 29, 2022, 11:45 a.m. UTC | #6
Hi Petr,

On 29-11-2022 11:01, Petr Vorel wrote:
> Hi Tudor,
> 
>> Accessing elements in an empty va_list results in undefined behaviour[0]
>> that can include accessing arbitrary stack memory. While typically this
>> doesn't raise a fault, some new more security-oriented architectures
>> (e.g. CHERI[1] or Morello[2]) don't allow it.
> 
>> Therefore, remove the variadicness from safe_* wrappers that always call
>> the functions with the optional argument included.
> 
>> Adapt the respective SAFE_* macros to handle the change by passing a
>> default argument if they're omitted.
> 
> Thanks for an interesting patchset!

Thank you for having a look!

> 
> I wonder if it's a correct approach as C API allows
> int open(const char *pathname, int flags);
> we will replace it
> int open(const char *pathname, int flags, mode_t mode);
> where mode is 0.

The man page for open specifies that in the cases where mode is ignored, 
it can either be omitted or specified as 0. safe_open was reading random 
bytes from the stack, while now it ensures it's 0 in those cases. I 
agree it's not a crystal clear approach for the same reasons you 
mention, but I find this approach to efficiently remove the undefined 
behaviour, while keeping the code clean.

> But as it's only in safe_* it should be ok.
> We still have some open() tests without mode, i.e.
> testcases/kernel/syscalls/open/open06.c

I don't think they need to be modified as a result of this patch series. 
It's up to the libcs to avoid the undefined behaviour in that case.

> 
> Unfortunately some tests need to be adjusted, at least all tests in
> testcases/kernel/syscalls/fgetxattr will fail due int-conversion,
> as they use NULL as the third parameter:
> 
> $ export CFLAGS="-Werror=conversion"
> $ ./configure
> $ cd testcases/kernel/syscalls/fgetxattr
> $ make clean
> $ make V=1
> gcc -I../../../../include -I../../../../include -I../../../../include/old/ -Werror=int-conversion -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -std=gnu99 -L../../../../lib fgetxattr01.c  -lltp -o fgetxattr01
> In file included from ../../../../include/tst_test.h:98,
>                   from fgetxattr01.c:34:
> fgetxattr01.c: In function ‘setup’:
> ../../../../include/tst_safe_macros.h:90:67: error: passing argument 6 of ‘safe_open’ makes integer from pointer without a cast [-Werror=int-conversion]
>     90 |         safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), (mode))
>        |                                                                   ^~~~~~
>        |                                                                   |
>        |                                                                   void *
> ../../../../include/tst_safe_macros.h:93:9: note: in expansion of macro ‘__SAFE_OPEN’
>     93 |         __SAFE_OPEN((pathname), (oflags), ##__VA_ARGS__, 0)
>        |         ^~~~~~~~~~~
> fgetxattr01.c:114:14: note: in expansion of macro ‘SAFE_OPEN’
>    114 |         fd = SAFE_OPEN(FNAME, O_RDONLY, NULL);
>        |              ^~~~~~~~~
> In file included from ../../../../include/tst_safe_macros.h:24:
> ../../../../include/safe_macros_fn.h:78:22: note: expected ‘mode_t’ {aka ‘unsigned int’} but argument is of type ‘void *’
>     78 |               mode_t mode);
>        |               ~~~~~~~^~~~
> cc1: some warnings being treated as errors
> make: *** [../../../../include/mk/rules.mk:43: fgetxattr01] Error 1
> 
> Found on recent openSUSE Tumbleweed which has 2.36. Also our CI has caught it on
> Fedora (which also uses 2.36):
> https://github.com/pevik/ltp/actions/runs/3573215390/jobs/6007016572

Many apologies for letting this slip through and many thanks for 
catching it! I'll remove the NULL and have a recheck on all the macros 
modified.

Kind regards,
Tudor

> 
> Kind regards,
> Petr
> 
>> [0]: [ISO/IEC 9899:2011] Programming Languages—C, 3rd ed, paragraph 7.16.1.1
>> [1]: https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/
>> [2]: https://www.morello-project.org/
> 
>> Tudor Cretu (3):
>>    safe_open: Fix undefined behaviour in vararg handling
>>    safe_openat: Fix undefined behaviour in vararg handling
>>    safe_semctl: Fix undefined behaviour in vararg handling
> 
>>   include/old/safe_macros.h   |  6 ++++--
>>   include/safe_macros_fn.h    |  3 ++-
>>   include/tst_safe_file_at.h  | 10 ++++++----
>>   include/tst_safe_macros.h   |  6 ++++--
>>   include/tst_safe_sysv_ipc.h | 14 +++++++++-----
>>   lib/safe_macros.c           | 13 +------------
>>   lib/tst_cgroup.c            |  2 +-
>>   lib/tst_safe_file_at.c      | 11 +++--------
>>   lib/tst_safe_sysv_ipc.c     | 10 +---------
>>   9 files changed, 31 insertions(+), 44 deletions(-)
Tudor Cretu Nov. 29, 2022, 11:46 a.m. UTC | #7
On 29-11-2022 11:32, Petr Vorel wrote:
>> Hello,
> 
>> This looks pretty trivial to fix actually. We shouldn't pass NULL as
>> mode. If it works I'll add the fix and push.
> Yes, it fixes it, good point. I was also surprised by NULL.

Many thanks, both! I'll remove the NULL and re-post a v2 if that's alright.

Kind regards,
Tudor

> 
> to whole patchset:
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> 
> Kind regards,
> Petr
Cyril Hrubis Nov. 29, 2022, 11:48 a.m. UTC | #8
Hi!
> Accessing elements in an empty va_list results in undefined behaviour[0]
> that can include accessing arbitrary stack memory. While typically this
> doesn't raise a fault, some new more security-oriented architectures
> (e.g. CHERI[1] or Morello[2]) don't allow it.

Looking at how glibc handles this, the code looks like:

	int mode = 0;

	if (__OPEN_NEEDS_MODE(oflag)) {
		..
		mode = va_arg(arg, int);
		..
	}

That sounds much easier than messing with the macros and should avoid
undefined behavior.
Richard Palethorpe Nov. 29, 2022, 12:01 p.m. UTC | #9
Hi,

Tudor Cretu <tudor.cretu@arm.com> writes:

> On 29-11-2022 11:32, Petr Vorel wrote:
>>> Hello,
>> 
>>> This looks pretty trivial to fix actually. We shouldn't pass NULL as
>>> mode. If it works I'll add the fix and push.
>> Yes, it fixes it, good point. I was also surprised by NULL.
>
> Many thanks, both! I'll remove the NULL and re-post a v2 if that's
> alright.

OK, please do.

>
> Kind regards,
> Tudor
>
>> to whole patchset:
>> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>> Kind regards,
>> Petr