* [LTP] [PATCH 1/3] safe_open: Fix undefined behaviour in vararg handling
2022-11-23 14:47 [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling Tudor Cretu
@ 2022-11-23 14:47 ` Tudor Cretu
2022-11-23 14:47 ` [LTP] [PATCH 2/3] safe_openat: " Tudor Cretu
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Tudor Cretu @ 2022-11-23 14:47 UTC (permalink / raw)
To: ltp
Accessing elements in an empty va_list is undefined behaviour.
Therefore, remove the variadicness from safe_open as it always calls
open with the mode argument included.
Adapt the SAFE_OPEN macro to handle the change by passing a default
argument of 0 to mode if it's omitted.
Signed-off-by: Tudor Cretu <tudor.cretu@arm.com>
---
include/old/safe_macros.h | 6 ++++--
include/safe_macros_fn.h | 3 ++-
include/tst_safe_macros.h | 6 ++++--
lib/safe_macros.c | 13 +------------
4 files changed, 11 insertions(+), 17 deletions(-)
diff --git a/include/old/safe_macros.h b/include/old/safe_macros.h
index fb1d7a110..d16540d63 100644
--- a/include/old/safe_macros.h
+++ b/include/old/safe_macros.h
@@ -59,9 +59,11 @@
#define SAFE_MUNMAP(cleanup_fn, addr, length) \
safe_munmap(__FILE__, __LINE__, (cleanup_fn), (addr), (length))
+#define __SAFE_OPEN(cleanup_fn, pathname, oflags, mode, ...) \
+ safe_open(__FILE__, __LINE__, (cleanup_fn), (pathname), (oflags), (mode))
+
#define SAFE_OPEN(cleanup_fn, pathname, oflags, ...) \
- safe_open(__FILE__, __LINE__, (cleanup_fn), (pathname), (oflags), \
- ##__VA_ARGS__)
+ __SAFE_OPEN((cleanup_fn), (pathname), (oflags), ##__VA_ARGS__, 0)
#define SAFE_PIPE(cleanup_fn, fildes) \
safe_pipe(__FILE__, __LINE__, cleanup_fn, (fildes))
diff --git a/include/safe_macros_fn.h b/include/safe_macros_fn.h
index 114d8fd43..d143079c3 100644
--- a/include/safe_macros_fn.h
+++ b/include/safe_macros_fn.h
@@ -74,7 +74,8 @@ int safe_munmap(const char *file, const int lineno,
void (*cleanup_fn)(void), void *addr, size_t length);
int safe_open(const char *file, const int lineno,
- void (*cleanup_fn)(void), const char *pathname, int oflags, ...);
+ void (*cleanup_fn)(void), const char *pathname, int oflags,
+ mode_t mode);
int safe_pipe(const char *file, const int lineno,
void (*cleanup_fn)(void), int fildes[2]);
diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index 81c4b0844..d53555c88 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -86,9 +86,11 @@ void *safe_realloc(const char *file, const int lineno, void *ptr, size_t size);
#define SAFE_MUNMAP(addr, length) \
safe_munmap(__FILE__, __LINE__, NULL, (addr), (length))
+#define __SAFE_OPEN(pathname, oflags, mode, ...) \
+ safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), (mode))
+
#define SAFE_OPEN(pathname, oflags, ...) \
- safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), \
- ##__VA_ARGS__)
+ __SAFE_OPEN((pathname), (oflags), ##__VA_ARGS__, 0)
#define SAFE_PIPE(fildes) \
safe_pipe(__FILE__, __LINE__, NULL, (fildes))
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index d8816631f..a92b58347 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -234,20 +234,9 @@ int safe_munmap(const char *file, const int lineno, void (*cleanup_fn) (void),
}
int safe_open(const char *file, const int lineno, void (*cleanup_fn) (void),
- const char *pathname, int oflags, ...)
+ const char *pathname, int oflags, mode_t mode)
{
- va_list ap;
int rval;
- mode_t mode;
-
- va_start(ap, oflags);
-
- /* Android's NDK's mode_t is smaller than an int, which results in
- * SIGILL here when passing the mode_t type.
- */
- mode = va_arg(ap, int);
-
- va_end(ap);
rval = open(pathname, oflags, mode);
--
2.25.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 13+ messages in thread* [LTP] [PATCH 2/3] safe_openat: Fix undefined behaviour in vararg handling
2022-11-23 14:47 [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling Tudor Cretu
2022-11-23 14:47 ` [LTP] [PATCH 1/3] safe_open: " Tudor Cretu
@ 2022-11-23 14:47 ` Tudor Cretu
2022-11-23 14:47 ` [LTP] [PATCH 3/3] safe_semctl: " Tudor Cretu
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Tudor Cretu @ 2022-11-23 14:47 UTC (permalink / raw)
To: ltp
Accessing elements in an empty va_list is undefined behaviour.
Therefore, remove the variadicness from safe_openat as it always calls
openat with the mode argument included.
Adapt the SAFE_OPENAT macro to handle the change by passing a default
argument of 0 to mode if it's omitted.
Signed-off-by: Tudor Cretu <tudor.cretu@arm.com>
---
include/tst_safe_file_at.h | 10 ++++++----
lib/tst_cgroup.c | 2 +-
lib/tst_safe_file_at.c | 11 +++--------
3 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/include/tst_safe_file_at.h b/include/tst_safe_file_at.h
index a1aa19fad..dd43d8f65 100644
--- a/include/tst_safe_file_at.h
+++ b/include/tst_safe_file_at.h
@@ -11,9 +11,11 @@
#include <unistd.h>
#include <stdarg.h>
-#define SAFE_OPENAT(dirfd, path, oflags, ...) \
- safe_openat(__FILE__, __LINE__, \
- (dirfd), (path), (oflags), ## __VA_ARGS__)
+#define __SAFE_OPENAT(dirfd, path, oflags, mode, ...) \
+ safe_openat(__FILE__, __LINE__, (dirfd), (path), (oflags), (mode))
+
+#define SAFE_OPENAT(dirfd, path, oflags, ...) \
+ __SAFE_OPENAT((dirfd), (path), (oflags), ##__VA_ARGS__, 0)
#define SAFE_FILE_READAT(dirfd, path, buf, nbyte) \
safe_file_readat(__FILE__, __LINE__, \
@@ -38,7 +40,7 @@ const char *tst_decode_fd(const int fd)
__attribute__((warn_unused_result));
int safe_openat(const char *const file, const int lineno, const int dirfd,
- const char *const path, const int oflags, ...)
+ const char *const path, const int oflags, const mode_t mode)
__attribute__((nonnull, warn_unused_result));
ssize_t safe_file_readat(const char *const file, const int lineno,
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 50699bc63..9831bc336 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -1345,7 +1345,7 @@ int safe_cg_open(const char *const file, const int lineno,
if (!alias)
continue;
- fds[i++] = safe_openat(file, lineno, (*dir)->dir_fd, alias, flags);
+ fds[i++] = safe_openat(file, lineno, (*dir)->dir_fd, alias, flags, 0);
}
return i;
diff --git a/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c
index f530dc349..9b8944f01 100644
--- a/lib/tst_safe_file_at.c
+++ b/lib/tst_safe_file_at.c
@@ -33,15 +33,10 @@ const char *tst_decode_fd(const int fd)
}
int safe_openat(const char *const file, const int lineno,
- const int dirfd, const char *const path, const int oflags, ...)
+ const int dirfd, const char *const path, const int oflags,
+ const mode_t mode)
{
- va_list ap;
int fd;
- mode_t mode;
-
- va_start(ap, oflags);
- mode = va_arg(ap, int);
- va_end(ap);
fd = openat(dirfd, path, oflags, mode);
if (fd > -1)
@@ -58,7 +53,7 @@ ssize_t safe_file_readat(const char *const file, const int lineno,
const int dirfd, const char *const path,
char *const buf, const size_t nbyte)
{
- int fd = safe_openat(file, lineno, dirfd, path, O_RDONLY);
+ int fd = safe_openat(file, lineno, dirfd, path, O_RDONLY, 0);
ssize_t rval;
if (fd < 0)
--
2.25.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 13+ messages in thread* [LTP] [PATCH 3/3] safe_semctl: Fix undefined behaviour in vararg handling
2022-11-23 14:47 [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling Tudor Cretu
2022-11-23 14:47 ` [LTP] [PATCH 1/3] safe_open: " Tudor Cretu
2022-11-23 14:47 ` [LTP] [PATCH 2/3] safe_openat: " Tudor Cretu
@ 2022-11-23 14:47 ` Tudor Cretu
2022-11-29 11:01 ` [LTP] [PATCH 0/3] safe_macros: " Petr Vorel
2022-11-29 11:48 ` Cyril Hrubis
4 siblings, 0 replies; 13+ messages in thread
From: Tudor Cretu @ 2022-11-23 14:47 UTC (permalink / raw)
To: ltp
Accessing elements in an empty va_list is undefined behaviour.
Therefore, remove the variadicness from safe_semctl as it always calls
semctl with the union semun argument included.
Adapt the SAFE_SEMCTL macro to handle the change by passing a
zero-initialised union semun if it's omitted.
Signed-off-by: Tudor Cretu <tudor.cretu@arm.com>
---
include/tst_safe_sysv_ipc.h | 14 +++++++++-----
lib/tst_safe_sysv_ipc.c | 10 +---------
2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/include/tst_safe_sysv_ipc.h b/include/tst_safe_sysv_ipc.h
index 7804ce192..976a30409 100644
--- a/include/tst_safe_sysv_ipc.h
+++ b/include/tst_safe_sysv_ipc.h
@@ -10,6 +10,7 @@
#include <sys/msg.h>
#include <sys/shm.h>
#include <sys/sem.h>
+#include "lapi/sem.h"
int safe_msgget(const char *file, const int lineno, key_t key, int msgflg);
#define SAFE_MSGGET(key, msgflg) \
@@ -58,11 +59,14 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems,
safe_semget(__FILE__, __LINE__, (key), (nsems), (semflg))
int safe_semctl(const char *file, const int lineno, int semid, int semnum,
- int cmd, ...);
-#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \
- int tst_ret_ = safe_semctl(__FILE__, __LINE__, (semid), (semnum), \
- (cmd), ##__VA_ARGS__); \
- (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \
+ int cmd, union semun un);
+#define __SAFE_SEMCTL(semid, semnum, cmd, un, ...) \
+ safe_semctl(__FILE__, __LINE__, (semid), (semnum), (cmd), (un))
+
+#define SAFE_SEMCTL(semid, semnum, cmd, ...) ({ \
+ int tst_ret_ = __SAFE_SEMCTL((semid), (semnum), (cmd), ##__VA_ARGS__, \
+ (union semun){0}); \
+ (semid) = ((cmd) == IPC_RMID ? -1 : (semid)); \
tst_ret_; })
int safe_semop(const char *file, const int lineno, int semid, struct sembuf *sops,
diff --git a/lib/tst_safe_sysv_ipc.c b/lib/tst_safe_sysv_ipc.c
index 5eaa82539..f99f6db5e 100644
--- a/lib/tst_safe_sysv_ipc.c
+++ b/lib/tst_safe_sysv_ipc.c
@@ -228,17 +228,9 @@ int safe_semget(const char *file, const int lineno, key_t key, int nsems,
}
int safe_semctl(const char *file, const int lineno, int semid, int semnum,
- int cmd, ...)
+ int cmd, union semun un)
{
int rval;
- va_list va;
- union semun un;
-
- va_start(va, cmd);
-
- un = va_arg(va, union semun);
-
- va_end(va);
rval = semctl(semid, semnum, cmd, un);
--
2.25.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-23 14:47 [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling Tudor Cretu
` (2 preceding siblings ...)
2022-11-23 14:47 ` [LTP] [PATCH 3/3] safe_semctl: " Tudor Cretu
@ 2022-11-29 11:01 ` Petr Vorel
2022-11-29 11:06 ` Petr Vorel
2022-11-29 11:45 ` Tudor Cretu
2022-11-29 11:48 ` Cyril Hrubis
4 siblings, 2 replies; 13+ messages in thread
From: Petr Vorel @ 2022-11-29 11:01 UTC (permalink / raw)
To: Tudor Cretu; +Cc: ltp, Richard Palethorpe
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(-)
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 11:01 ` [LTP] [PATCH 0/3] safe_macros: " Petr Vorel
@ 2022-11-29 11:06 ` Petr Vorel
2022-11-29 11:11 ` Richard Palethorpe
2022-11-29 11:45 ` Tudor Cretu
1 sibling, 1 reply; 13+ messages in thread
From: Petr Vorel @ 2022-11-29 11:06 UTC (permalink / raw)
To: Tudor Cretu, ltp, Cyril Hrubis, Richard Palethorpe
> 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(-)
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 11:06 ` Petr Vorel
@ 2022-11-29 11:11 ` Richard Palethorpe
2022-11-29 11:27 ` Richard Palethorpe
0 siblings, 1 reply; 13+ messages in thread
From: Richard Palethorpe @ 2022-11-29 11:11 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 11:11 ` Richard Palethorpe
@ 2022-11-29 11:27 ` Richard Palethorpe
2022-11-29 11:32 ` Petr Vorel
0 siblings, 1 reply; 13+ messages in thread
From: Richard Palethorpe @ 2022-11-29 11:27 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 11:27 ` Richard Palethorpe
@ 2022-11-29 11:32 ` Petr Vorel
2022-11-29 11:46 ` Tudor Cretu
0 siblings, 1 reply; 13+ messages in thread
From: Petr Vorel @ 2022-11-29 11:32 UTC (permalink / raw)
To: Richard Palethorpe; +Cc: ltp
> 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
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 11:32 ` Petr Vorel
@ 2022-11-29 11:46 ` Tudor Cretu
2022-11-29 12:01 ` Richard Palethorpe
0 siblings, 1 reply; 13+ messages in thread
From: Tudor Cretu @ 2022-11-29 11:46 UTC (permalink / raw)
To: Petr Vorel, Richard Palethorpe; +Cc: ltp
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
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 11:46 ` Tudor Cretu
@ 2022-11-29 12:01 ` Richard Palethorpe
0 siblings, 0 replies; 13+ messages in thread
From: Richard Palethorpe @ 2022-11-29 12:01 UTC (permalink / raw)
To: Tudor Cretu; +Cc: ltp
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
--
Thank you,
Richard.
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 11:01 ` [LTP] [PATCH 0/3] safe_macros: " Petr Vorel
2022-11-29 11:06 ` Petr Vorel
@ 2022-11-29 11:45 ` Tudor Cretu
1 sibling, 0 replies; 13+ messages in thread
From: Tudor Cretu @ 2022-11-29 11:45 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp, Richard Palethorpe
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(-)
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-23 14:47 [LTP] [PATCH 0/3] safe_macros: Fix undefined behaviour in vararg handling Tudor Cretu
` (3 preceding siblings ...)
2022-11-29 11:01 ` [LTP] [PATCH 0/3] safe_macros: " Petr Vorel
@ 2022-11-29 11:48 ` Cyril Hrubis
4 siblings, 0 replies; 13+ messages in thread
From: Cyril Hrubis @ 2022-11-29 11:48 UTC (permalink / raw)
To: Tudor Cretu; +Cc: ltp
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.
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 13+ messages in thread