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

[Tudor Cretu]

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