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

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

[Tudor Cretu]

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

[LTP] [PATCH v2 1/3] safe_open: Fix undefined behaviour in vararg handling

[Tudor Cretu]

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

[LTP] [PATCH v2 2/3] safe_openat: Fix undefined behaviour in vararg handling

[Tudor Cretu]

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

[LTP] [PATCH v2 3/3] safe_semctl: Fix undefined behaviour in vararg handling

[Tudor Cretu]

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

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

[Richard Palethorpe]

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

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

[Petr Vorel]

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

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

[Tudor Cretu]

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

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

[Petr Vorel]

> 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

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

[Tudor Cretu]

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