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

[LTP] [PATCH v4 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.

[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/

v4..v3:
  - Renamed __OPEN_NEEDS_MODE to TST_OPEN_NEEDS_MODE

v3..v2:
  - Separate the f{get,set}xattr changes into a new patch: PATCH 1/3
  - Don't remove the variadicness from safe_* wrappers anymore, but only
    read the variadic arguments in the cases where it's expected
  - Remove the changes to SAFE_* macros as they're not needed anymore
  - Add include/lapi/ipc.h to define IPC_INFO
  - define __OPEN_NEEDS_MODE similar to the conditions used in supported
    libcs (glibc, musl, uclibc, and Bionic), to be shared by both
    safe_open and safe_openat.
  - The switch case in safe_semctl is the same as the one used in glibc,
    and functionally similar to the one used in Musl. Bionic and uclibc
    don't have a similar switch case, they read the union semun vararg
    unconditionally.
  - Tested with both glibc and Musl

v2..v1:
  - PATCH 1: Remove the NULL argument for mode from SAFE_OPEN instances
    to avoid the pointer to int conversion.

Tudor Cretu (3):
  syscalls/f{get,set}xattr: Don't pass a pointer to mode argument in
    open
  safe_open, safe_openat: Fix undefined behaviour in vararg handling
  safe_semctl: Fix undefined behaviour in vararg handling

 include/lapi/fcntl.h                          |  3 +++
 include/lapi/ipc.h                            | 14 +++++++++++
 lib/safe_macros.c                             | 21 +++++++++--------
 lib/tst_safe_file_at.c                        | 13 +++++++----
 lib/tst_safe_sysv_ipc.c                       | 23 +++++++++++++------
 .../kernel/syscalls/fgetxattr/fgetxattr01.c   |  2 +-
 .../kernel/syscalls/fgetxattr/fgetxattr02.c   |  2 +-
 .../kernel/syscalls/fgetxattr/fgetxattr03.c   |  2 +-
 .../kernel/syscalls/fsetxattr/fsetxattr01.c   |  2 +-
 .../kernel/syscalls/fsetxattr/fsetxattr02.c   |  2 +-
 10 files changed, 58 insertions(+), 26 deletions(-)
 create mode 100644 include/lapi/ipc.h

-- 
2.25.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4 1/3] syscalls/f{get, set}xattr: Don't pass a pointer to mode argument in open

[Tudor Cretu]

Passing NULL to open as the mode parameter is undefined behaviour
as a mode_t is a numerical type, not compatible with a pointer type.
If neither O_CREAT nor O_TMPFILE is specified in flags, then the mode
argument is ignored, so just remove it in these cases.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Tudor Cretu <tudor.cretu@arm.com>
---
 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 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

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 v4 2/3] safe_open, safe_openat: Fix undefined behaviour in vararg handling

[Tudor Cretu]

Accessing elements in an empty va_list is undefined behaviour.
The open system call expects the mode argument only when O_CREAT or
O_TMPFILE is specified in flags, otherwise the mode argument is ignored.

Modify the safe_open and safe_openat wrappers to read the variadic
argument only when it's expected to be provided.

Signed-off-by: Tudor Cretu <tudor.cretu@arm.com>
---
 include/lapi/fcntl.h   |  3 +++
 lib/safe_macros.c      | 21 ++++++++++++---------
 lib/tst_safe_file_at.c | 13 ++++++++-----
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/include/lapi/fcntl.h b/include/lapi/fcntl.h
index 8a0a893f9..f17220737 100644
--- a/include/lapi/fcntl.h
+++ b/include/lapi/fcntl.h
@@ -141,6 +141,9 @@
 # define MAX_HANDLE_SZ	128
 #endif
 
+#define TST_OPEN_NEEDS_MODE(oflag) \
+	(((oflag) & O_CREAT) != 0 || ((oflag) & O_TMPFILE) == O_TMPFILE)
+
 #ifndef HAVE_STRUCT_FILE_HANDLE
 struct file_handle {
 	unsigned int handle_bytes;
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index d8816631f..d34b71fda 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -13,7 +13,6 @@
 #include <sys/xattr.h>
 #include <sys/sysinfo.h>
 #include <errno.h>
-#include <fcntl.h>
 #include <libgen.h>
 #include <limits.h>
 #include <pwd.h>
@@ -21,6 +20,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <malloc.h>
+#include "lapi/fcntl.h"
 #include "test.h"
 #include "safe_macros.h"
 
@@ -236,18 +236,21 @@ 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, ...)
 {
-	va_list ap;
 	int rval;
-	mode_t mode;
+	mode_t mode = 0;
 
-	va_start(ap, oflags);
+	if (TST_OPEN_NEEDS_MODE(oflags)) {
+		va_list ap;
 
-	/* 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_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);
+		va_end(ap);
+	}
 
 	rval = open(pathname, oflags, mode);
 
diff --git a/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c
index f530dc349..257f70291 100644
--- a/lib/tst_safe_file_at.c
+++ b/lib/tst_safe_file_at.c
@@ -35,13 +35,16 @@ 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, ...)
 {
-	va_list ap;
 	int fd;
-	mode_t mode;
+	mode_t mode = 0;
 
-	va_start(ap, oflags);
-	mode = va_arg(ap, int);
-	va_end(ap);
+	if (TST_OPEN_NEEDS_MODE(oflags)) {
+		va_list ap;
+
+		va_start(ap, oflags);
+		mode = va_arg(ap, int);
+		va_end(ap);
+	}
 
 	fd = openat(dirfd, path, oflags, mode);
 	if (fd > -1)
-- 
2.25.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

[Tudor Cretu]

Accessing elements in an empty va_list is undefined behaviour.
The semctl system call expects the union semun argument only for a
set of cmd values, otherwise the argument is ignored.

Modify the safe_semctl wrapper to read the variadic argument only when
it's expected to be provided.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Tudor Cretu <tudor.cretu@arm.com>
---
 include/lapi/ipc.h      | 14 ++++++++++++++
 lib/tst_safe_sysv_ipc.c | 23 ++++++++++++++++-------
 2 files changed, 30 insertions(+), 7 deletions(-)
 create mode 100644 include/lapi/ipc.h

diff --git a/include/lapi/ipc.h b/include/lapi/ipc.h
new file mode 100644
index 000000000..5645c8817
--- /dev/null
+++ b/include/lapi/ipc.h
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Arm Ltd.
+ */
+#ifndef LAPI_IPC_H__
+#define LAPI_IPC_H__
+
+#include <sys/ipc.h>
+
+#ifndef IPC_INFO
+# define IPC_INFO 3
+#endif
+
+#endif /* LAPI_IPC_H__ */
diff --git a/lib/tst_safe_sysv_ipc.c b/lib/tst_safe_sysv_ipc.c
index 5eaa82539..a196fc9ce 100644
--- a/lib/tst_safe_sysv_ipc.c
+++ b/lib/tst_safe_sysv_ipc.c
@@ -4,12 +4,12 @@
  */
 
 #include <sys/types.h>
-#include <sys/ipc.h>
 #include <sys/msg.h>
 #include <sys/shm.h>
 #define TST_NO_DEFAULT_MAIN
 #include "tst_test.h"
 #include "tst_safe_sysv_ipc.h"
+#include "lapi/ipc.h"
 #include "lapi/sem.h"
 
 /*
@@ -232,13 +232,22 @@ int safe_semctl(const char *file, const int lineno, int semid, int semnum,
 {
 	int rval;
 	va_list va;
-	union semun un;
+	union semun un = {0};
 
-	va_start(va, cmd);
-
-	un = va_arg(va, union semun);
-
-	va_end(va);
+	switch (cmd) {
+	case SETVAL:
+	case GETALL:
+	case SETALL:
+	case IPC_STAT:
+	case IPC_SET:
+	case SEM_STAT:
+	case SEM_STAT_ANY:
+	case IPC_INFO:
+	case SEM_INFO:
+		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 v4 2/3] safe_open, safe_openat: Fix undefined behaviour in vararg handling

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4 2/3] safe_open, safe_openat: Fix undefined behaviour in vararg handling

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
>
> -- 
> Cyril Hrubis
> chrubis@suse.cz

Still not sure I prefer this over the macro approach, but merged,
thanks!


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp