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

[Richard Palethorpe <rpalethorpe@suse.de>]

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(-)


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp