* [LTP] [PATCH v2 1/3] safe_open: Fix undefined behaviour in vararg handling
2022-11-29 13:03 [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling Tudor Cretu
@ 2022-11-29 13:03 ` Tudor Cretu
2022-11-29 13:03 ` [LTP] [PATCH v2 2/3] safe_openat: " Tudor Cretu
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Tudor Cretu @ 2022-11-29 13:03 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.
Some instances of SAFE_OPEN were passing NULL as the mode argument
instead of an int. Remove it and allow the macros to handle it to avoid
a pointer to int conversion.
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 +------------
testcases/kernel/syscalls/fgetxattr/fgetxattr01.c | 2 +-
testcases/kernel/syscalls/fgetxattr/fgetxattr02.c | 2 +-
testcases/kernel/syscalls/fgetxattr/fgetxattr03.c | 2 +-
testcases/kernel/syscalls/fsetxattr/fsetxattr01.c | 2 +-
testcases/kernel/syscalls/fsetxattr/fsetxattr02.c | 2 +-
9 files changed, 16 insertions(+), 22 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);
diff --git a/testcases/kernel/syscalls/fgetxattr/fgetxattr01.c b/testcases/kernel/syscalls/fgetxattr/fgetxattr01.c
index 35c46a1c3..52e6e44ab 100644
--- a/testcases/kernel/syscalls/fgetxattr/fgetxattr01.c
+++ b/testcases/kernel/syscalls/fgetxattr/fgetxattr01.c
@@ -111,7 +111,7 @@ static void setup(void)
size_t i = 0;
SAFE_TOUCH(FNAME, 0644, NULL);
- fd = SAFE_OPEN(FNAME, O_RDONLY, NULL);
+ fd = SAFE_OPEN(FNAME, O_RDONLY);
for (i = 0; i < ARRAY_SIZE(tc); i++) {
tc[i].value = SAFE_MALLOC(tc[i].size);
diff --git a/testcases/kernel/syscalls/fgetxattr/fgetxattr02.c b/testcases/kernel/syscalls/fgetxattr/fgetxattr02.c
index 82fb676be..c3cff0aab 100644
--- a/testcases/kernel/syscalls/fgetxattr/fgetxattr02.c
+++ b/testcases/kernel/syscalls/fgetxattr/fgetxattr02.c
@@ -244,7 +244,7 @@ static void setup(void)
SAFE_BIND(tc[i].fd, (const struct sockaddr *) &sun,
sizeof(struct sockaddr_un));
} else {
- tc[i].fd = SAFE_OPEN(tc[i].fname, tc[i].fflags, NULL);
+ tc[i].fd = SAFE_OPEN(tc[i].fname, tc[i].fflags);
}
if (tc[i].exp_ret >= 0) {
diff --git a/testcases/kernel/syscalls/fgetxattr/fgetxattr03.c b/testcases/kernel/syscalls/fgetxattr/fgetxattr03.c
index d293ffca9..0581b9670 100644
--- a/testcases/kernel/syscalls/fgetxattr/fgetxattr03.c
+++ b/testcases/kernel/syscalls/fgetxattr/fgetxattr03.c
@@ -48,7 +48,7 @@ static void verify_fgetxattr(void)
static void setup(void)
{
SAFE_TOUCH(FILENAME, 0644, NULL);
- fd = SAFE_OPEN(FILENAME, O_RDONLY, NULL);
+ fd = SAFE_OPEN(FILENAME, O_RDONLY);
SAFE_FSETXATTR(fd, XATTR_TEST_KEY, XATTR_TEST_VALUE,
XATTR_TEST_VALUE_SIZE, XATTR_CREATE);
diff --git a/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c b/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c
index ffec8104f..d799e477f 100644
--- a/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c
+++ b/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c
@@ -205,7 +205,7 @@ static void setup(void)
long_value[XATTR_SIZE_MAX + 1] = '\0';
SAFE_TOUCH(FNAME, 0644, NULL);
- fd = SAFE_OPEN(FNAME, O_RDONLY, NULL);
+ fd = SAFE_OPEN(FNAME, O_RDONLY);
for (i = 0; i < ARRAY_SIZE(tc); i++) {
if (!tc[i].key)
diff --git a/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c b/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c
index 3aea4b59e..0336c964a 100644
--- a/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c
+++ b/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c
@@ -211,7 +211,7 @@ static void setup(void)
for (i = 0; i < ARRAY_SIZE(tc); i++) {
if (!tc[i].issocket) {
- tc[i].fd = SAFE_OPEN(tc[i].fname, tc[i].fflags, NULL);
+ tc[i].fd = SAFE_OPEN(tc[i].fname, tc[i].fflags);
continue;
}
--
2.25.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 9+ messages in thread* [LTP] [PATCH v2 2/3] safe_openat: Fix undefined behaviour in vararg handling
2022-11-29 13:03 [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling Tudor Cretu
2022-11-29 13:03 ` [LTP] [PATCH v2 1/3] safe_open: " Tudor Cretu
@ 2022-11-29 13:03 ` Tudor Cretu
2022-11-29 13:03 ` [LTP] [PATCH v2 3/3] safe_semctl: " Tudor Cretu
2022-11-29 13:23 ` [LTP] [PATCH v2 0/3] safe_macros: " Richard Palethorpe
3 siblings, 0 replies; 9+ messages in thread
From: Tudor Cretu @ 2022-11-29 13:03 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] 9+ messages in thread* [LTP] [PATCH v2 3/3] safe_semctl: Fix undefined behaviour in vararg handling
2022-11-29 13:03 [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling Tudor Cretu
2022-11-29 13:03 ` [LTP] [PATCH v2 1/3] safe_open: " Tudor Cretu
2022-11-29 13:03 ` [LTP] [PATCH v2 2/3] safe_openat: " Tudor Cretu
@ 2022-11-29 13:03 ` Tudor Cretu
2022-11-29 13:23 ` [LTP] [PATCH v2 0/3] safe_macros: " Richard Palethorpe
3 siblings, 0 replies; 9+ messages in thread
From: Tudor Cretu @ 2022-11-29 13:03 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] 9+ messages in thread* Re: [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 13:03 [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling Tudor Cretu
` (2 preceding siblings ...)
2022-11-29 13:03 ` [LTP] [PATCH v2 3/3] safe_semctl: " Tudor Cretu
@ 2022-11-29 13:23 ` Richard Palethorpe
2022-11-29 13:59 ` Petr Vorel
3 siblings, 1 reply; 9+ messages in thread
From: Richard Palethorpe @ 2022-11-29 13:23 UTC (permalink / raw)
To: Tudor Cretu; +Cc: ltp
Hello,
So I'm happy with this, but I think Cyril's comment deserves a response:
> 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.
I don't see why, __OPEN_NEEDS_MODE is going to be different between
functions and libc/kernel versions.
Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
Tudor Cretu <tudor.cretu@arm.com> writes:
> 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/
>
> v2..v1:
> - PATCH 1: Remove the NULL argument for mode from SAFE_OPEN instances
> to avoid the pointer to int conversion.
>
> 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 +---------
> testcases/kernel/syscalls/fgetxattr/fgetxattr01.c | 2 +-
> testcases/kernel/syscalls/fgetxattr/fgetxattr02.c | 2 +-
> testcases/kernel/syscalls/fgetxattr/fgetxattr03.c | 2 +-
> testcases/kernel/syscalls/fsetxattr/fsetxattr01.c | 2 +-
> testcases/kernel/syscalls/fsetxattr/fsetxattr02.c | 2 +-
> 14 files changed, 36 insertions(+), 49 deletions(-)
>
> --
> 2.25.1
--
Thank you,
Richard.
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 13:23 ` [LTP] [PATCH v2 0/3] safe_macros: " Richard Palethorpe
@ 2022-11-29 13:59 ` Petr Vorel
2022-11-29 14:04 ` Tudor Cretu
0 siblings, 1 reply; 9+ messages in thread
From: Petr Vorel @ 2022-11-29 13:59 UTC (permalink / raw)
To: Richard Palethorpe; +Cc: ltp
Hi all,
> Hello,
> So I'm happy with this, but I think Cyril's comment deserves a response:
+1
> > 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.
+1
> I don't see why, __OPEN_NEEDS_MODE is going to be different between
> functions and libc/kernel versions.
Looking at glibc's __OPEN_NEEDS_MODE definition, the logic is obviously the same
as musl code for open (it just use O_TMPFILE instead of __O_TMPFILE therefore no
need to check for #ifdef __O_TMPFILE).
Kind regards,
Petr
> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
> Tudor Cretu <tudor.cretu@arm.com> writes:
> > 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/
> > v2..v1:
> > - PATCH 1: Remove the NULL argument for mode from SAFE_OPEN instances
> > to avoid the pointer to int conversion.
> > 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 +---------
> > testcases/kernel/syscalls/fgetxattr/fgetxattr01.c | 2 +-
> > testcases/kernel/syscalls/fgetxattr/fgetxattr02.c | 2 +-
> > testcases/kernel/syscalls/fgetxattr/fgetxattr03.c | 2 +-
> > testcases/kernel/syscalls/fsetxattr/fsetxattr01.c | 2 +-
> > testcases/kernel/syscalls/fsetxattr/fsetxattr02.c | 2 +-
> > 14 files changed, 36 insertions(+), 49 deletions(-)
> > --
> > 2.25.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 13:59 ` Petr Vorel
@ 2022-11-29 14:04 ` Tudor Cretu
2022-11-29 15:15 ` Petr Vorel
0 siblings, 1 reply; 9+ messages in thread
From: Tudor Cretu @ 2022-11-29 14:04 UTC (permalink / raw)
To: Petr Vorel, Richard Palethorpe; +Cc: ltp
On 29-11-2022 13:59, Petr Vorel wrote:
> Hi all,
>
>> Hello,
>
>> So I'm happy with this, but I think Cyril's comment deserves a response:
Indeed, I noticed it too late after sending the v2.
> +1
>
>>> 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.
I considered this and I think it's better to focus strictly on the
handling the variadicness issue, and wanted to avoid duplicating logic
from libcs.
> +1
>
>> I don't see why, __OPEN_NEEDS_MODE is going to be different between
>
>> functions and libc/kernel versions.
Haven't thought about that, that's a good point in my opinion.
>
> Looking at glibc's __OPEN_NEEDS_MODE definition, the logic is obviously the same
> as musl code for open (it just use O_TMPFILE instead of __O_TMPFILE therefore no
> need to check for #ifdef __O_TMPFILE).
I agree, for open/openat this approach would be fairly simple, there is
semctl too though, I'll need to have a look how glibc and musl handle it.
Kind regards,
Tudor
>
> Kind regards,
> Petr
>
>> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
>
>> Tudor Cretu <tudor.cretu@arm.com> writes:
>
>>> 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/
>
>>> v2..v1:
>>> - PATCH 1: Remove the NULL argument for mode from SAFE_OPEN instances
>>> to avoid the pointer to int conversion.
>
>>> 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 +---------
>>> testcases/kernel/syscalls/fgetxattr/fgetxattr01.c | 2 +-
>>> testcases/kernel/syscalls/fgetxattr/fgetxattr02.c | 2 +-
>>> testcases/kernel/syscalls/fgetxattr/fgetxattr03.c | 2 +-
>>> testcases/kernel/syscalls/fsetxattr/fsetxattr01.c | 2 +-
>>> testcases/kernel/syscalls/fsetxattr/fsetxattr02.c | 2 +-
>>> 14 files changed, 36 insertions(+), 49 deletions(-)
>
>>> --
>>> 2.25.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 14:04 ` Tudor Cretu
@ 2022-11-29 15:15 ` Petr Vorel
2022-11-30 13:43 ` Tudor Cretu
0 siblings, 1 reply; 9+ messages in thread
From: Petr Vorel @ 2022-11-29 15:15 UTC (permalink / raw)
To: Tudor Cretu; +Cc: ltp
> On 29-11-2022 13:59, Petr Vorel wrote:
> > Hi all,
> > > Hello,
> > > So I'm happy with this, but I think Cyril's comment deserves a response:
> Indeed, I noticed it too late after sending the v2.
> > +1
> > > > 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.
> I considered this and I think it's better to focus strictly on the handling
> the variadicness issue, and wanted to avoid duplicating logic from libcs.
> > +1
> > > I don't see why, __OPEN_NEEDS_MODE is going to be different between
> > > functions and libc/kernel versions.
> Haven't thought about that, that's a good point in my opinion.
> > Looking at glibc's __OPEN_NEEDS_MODE definition, the logic is obviously the same
> > as musl code for open (it just use O_TMPFILE instead of __O_TMPFILE therefore no
> > need to check for #ifdef __O_TMPFILE).
> I agree, for open/openat this approach would be fairly simple, there is
> semctl too though, I'll need to have a look how glibc and musl handle it.
Thanks a lot for your time Tudor!
Kind regards,
Petr
> Kind regards,
> Tudor
> > Kind regards,
> > Petr
> > > Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
> > > Tudor Cretu <tudor.cretu@arm.com> writes:
> > > > 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/
> > > > v2..v1:
> > > > - PATCH 1: Remove the NULL argument for mode from SAFE_OPEN instances
> > > > to avoid the pointer to int conversion.
> > > > 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 +---------
> > > > testcases/kernel/syscalls/fgetxattr/fgetxattr01.c | 2 +-
> > > > testcases/kernel/syscalls/fgetxattr/fgetxattr02.c | 2 +-
> > > > testcases/kernel/syscalls/fgetxattr/fgetxattr03.c | 2 +-
> > > > testcases/kernel/syscalls/fsetxattr/fsetxattr01.c | 2 +-
> > > > testcases/kernel/syscalls/fsetxattr/fsetxattr02.c | 2 +-
> > > > 14 files changed, 36 insertions(+), 49 deletions(-)
> > > > --
> > > > 2.25.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling
2022-11-29 15:15 ` Petr Vorel
@ 2022-11-30 13:43 ` Tudor Cretu
0 siblings, 0 replies; 9+ messages in thread
From: Tudor Cretu @ 2022-11-30 13:43 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp
On 29-11-2022 15:15, Petr Vorel wrote:
>
>
>> On 29-11-2022 13:59, Petr Vorel wrote:
>>> Hi all,
>
>>>> Hello,
>
>>>> So I'm happy with this, but I think Cyril's comment deserves a response:
>
>> Indeed, I noticed it too late after sending the v2.
>
>>> +1
>
>>>>> 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.
>
>> I considered this and I think it's better to focus strictly on the handling
>> the variadicness issue, and wanted to avoid duplicating logic from libcs.
>
>>> +1
>
>>>> I don't see why, __OPEN_NEEDS_MODE is going to be different between
>
>>>> functions and libc/kernel versions.
>
>> Haven't thought about that, that's a good point in my opinion.
>
>
>>> Looking at glibc's __OPEN_NEEDS_MODE definition, the logic is obviously the same
>>> as musl code for open (it just use O_TMPFILE instead of __O_TMPFILE therefore no
>>> need to check for #ifdef __O_TMPFILE).
>
>> I agree, for open/openat this approach would be fairly simple, there is
>> semctl too though, I'll need to have a look how glibc and musl handle it.
>
> Thanks a lot for your time Tudor!
My pleasure! Many thanks for the feedback, all. I have posted a v3. I
look forward to your opinions.
Kind regards,
Tudor
>
> Kind regards,
> Petr
>
>> Kind regards,
>> Tudor
>
>
>>> Kind regards,
>>> Petr
>
>>>> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
>
>>>> Tudor Cretu <tudor.cretu@arm.com> writes:
>
>>>>> 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/
>
>>>>> v2..v1:
>>>>> - PATCH 1: Remove the NULL argument for mode from SAFE_OPEN instances
>>>>> to avoid the pointer to int conversion.
>
>>>>> 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 +---------
>>>>> testcases/kernel/syscalls/fgetxattr/fgetxattr01.c | 2 +-
>>>>> testcases/kernel/syscalls/fgetxattr/fgetxattr02.c | 2 +-
>>>>> testcases/kernel/syscalls/fgetxattr/fgetxattr03.c | 2 +-
>>>>> testcases/kernel/syscalls/fsetxattr/fsetxattr01.c | 2 +-
>>>>> testcases/kernel/syscalls/fsetxattr/fsetxattr02.c | 2 +-
>>>>> 14 files changed, 36 insertions(+), 49 deletions(-)
>
>>>>> --
>>>>> 2.25.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 9+ messages in thread