| Message ID | 20221123144746.590890-1-tudor.cretu@arm.com |
|---|---|
| Headers | show |
| Series | safe_macros: Fix undefined behaviour in vararg handling | expand |
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(-)
> 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(-)
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(-)
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(-)
> 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
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(-)
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
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.
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