[LTP] [PATCH v2 0/4] Improve reliability of ioctl_sg01

[LTP] [PATCH v2 0/4] Improve reliability of ioctl_sg01

[Martin Doucha]

ioctl_sg01 needs dirty memory to reliably detect kernel data leaks. Its
description originally recommended running it after mem01 which has been
recently dropped.

Add helper function to LTP library for pre-polluting free memory and loop
the main check in ioctl_sg01 to further decrease the chance of false negative.

Martin Doucha (4):
  Add SAFE_SYSINFO() macro
  Add tst_pollute_memory() helper function
  ioctl_sg01: Pollute free memory in setup
  ioctl_sg01: Loop data leak check 100 times

 include/tst_memutils.h                       | 22 +++++++
 include/tst_safe_macros.h                    |  5 ++
 lib/safe_macros.c                            | 20 +++++++
 lib/tst_memutils.c                           | 62 ++++++++++++++++++++
 testcases/kernel/syscalls/ioctl/ioctl_sg01.c | 32 ++++++----
 5 files changed, 128 insertions(+), 13 deletions(-)
 create mode 100644 include/tst_memutils.h
 create mode 100644 lib/tst_memutils.c

-- 
2.28.0

[LTP] [PATCH v2 1/4] Add SAFE_SYSINFO() macro

[Martin Doucha]

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1: New patch

 include/tst_safe_macros.h |  5 +++++
 lib/safe_macros.c         | 20 ++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index 000381c4f..053c3bcf9 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -12,6 +12,7 @@
 #include <sys/resource.h>
 #include <sys/stat.h>
 #include <sys/vfs.h>
+#include <sys/sysinfo.h>
 #include <fcntl.h>
 #include <libgen.h>
 #include <signal.h>
@@ -598,4 +599,8 @@ long tst_safe_ptrace(const char *file, const int lineno, int req, pid_t pid,
 #define SAFE_PTRACE(req, pid, addr, data) \
 	tst_safe_ptrace(__FILE__, __LINE__, req, pid, addr, data)
 
+int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info);
+#define SAFE_SYSINFO(info) \
+	safe_sysinfo(__FILE__, __LINE__, (info))
+
 #endif /* SAFE_MACROS_H__ */
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index dde9b7b5e..5ef9ee1c5 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -11,6 +11,7 @@
 #include <sys/wait.h>
 #include <sys/mount.h>
 #include <sys/xattr.h>
+#include <sys/sysinfo.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <libgen.h>
@@ -1088,3 +1089,22 @@ int safe_mincore(const char *file, const int lineno, void *start,
 
 	return rval;
 }
+
+int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info)
+{
+	int ret;
+
+	errno = 0;
+	ret = sysinfo(info);
+
+	if (ret == -1) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
+			"sysinfo() failed");
+	} else if (ret) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
+			"Invalid sysinfo() return value %d", ret);
+
+	}
+
+	return ret;
+}
-- 
2.28.0

[LTP] [PATCH v2 1/4] Add SAFE_SYSINFO() macro

[Petr Vorel]

Hi Martin,

> Changes since v1: New patch

>  include/tst_safe_macros.h |  5 +++++
>  lib/safe_macros.c         | 20 ++++++++++++++++++++
>  2 files changed, 25 insertions(+)

> diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
> index 000381c4f..053c3bcf9 100644
> --- a/include/tst_safe_macros.h
> +++ b/include/tst_safe_macros.h
> @@ -12,6 +12,7 @@
>  #include <sys/resource.h>
>  #include <sys/stat.h>
>  #include <sys/vfs.h>
> +#include <sys/sysinfo.h>
>  #include <fcntl.h>
>  #include <libgen.h>
>  #include <signal.h>
> @@ -598,4 +599,8 @@ long tst_safe_ptrace(const char *file, const int lineno, int req, pid_t pid,
>  #define SAFE_PTRACE(req, pid, addr, data) \
>  	tst_safe_ptrace(__FILE__, __LINE__, req, pid, addr, data)

> +int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info);
> +#define SAFE_SYSINFO(info) \
> +	safe_sysinfo(__FILE__, __LINE__, (info))
> +
>  #endif /* SAFE_MACROS_H__ */
> diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> index dde9b7b5e..5ef9ee1c5 100644
> --- a/lib/safe_macros.c
> +++ b/lib/safe_macros.c
> @@ -11,6 +11,7 @@
>  #include <sys/wait.h>
>  #include <sys/mount.h>
>  #include <sys/xattr.h>
> +#include <sys/sysinfo.h>
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <libgen.h>
> @@ -1088,3 +1089,22 @@ int safe_mincore(const char *file, const int lineno, void *start,

>  	return rval;
>  }
> +
> +int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info)
> +{
> +	int ret;
> +
> +	errno = 0;
> +	ret = sysinfo(info);
> +
> +	if (ret == -1) {
> +		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
> +			"sysinfo() failed");
> +	} else if (ret) {
> +		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
> +			"Invalid sysinfo() return value %d", ret);
nit: unneeded line below.
> +
> +	}
Man page mentions only 0 and -1. Sure, it does not harm to test ret > 0, I just
wonder if it's needed.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> +
> +	return ret;
> +}

[LTP] [PATCH v2 2/4] Add tst_pollute_memory() helper function

[Martin Doucha]

tst_pollute_memory() fills available RAM up to specified limit with given fill
byte. Useful for testing data disclosure vulnerablities.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1: New patch

 include/tst_memutils.h | 22 +++++++++++++++
 lib/tst_memutils.c     | 62 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)
 create mode 100644 include/tst_memutils.h
 create mode 100644 lib/tst_memutils.c

diff --git a/include/tst_memutils.h b/include/tst_memutils.h
new file mode 100644
index 000000000..91dad07cd
--- /dev/null
+++ b/include/tst_memutils.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2020 SUSE LLC <mdoucha@suse.cz>
+ */
+
+#ifndef TST_MEMUTILS_H__
+#define TST_MEMUTILS_H__
+
+/*
+ * Fill up to maxsize physical memory with fillchar, then free it for reuse.
+ * If maxsize is zero, fill as much memory as possible. This function is
+ * intended for data disclosure vulnerability tests to reduce the probability
+ * that a vulnerable kernel will leak a block of memory that was full of
+ * zeroes by chance.
+ *
+ * The function keeps a safety margin to avoid invoking OOM killer and
+ * respects the limitations of available address space. (Less than 3GB can be
+ * polluted on a 32bit system regardless of available physical RAM.)
+ */
+void tst_pollute_memory(size_t maxsize, int fillchar);
+
+#endif /* TST_MEMUTILS_H__ */
diff --git a/lib/tst_memutils.c b/lib/tst_memutils.c
new file mode 100644
index 000000000..f134d90c9
--- /dev/null
+++ b/lib/tst_memutils.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 SUSE LLC <mdoucha@suse.cz>
+ */
+
+#include <unistd.h>
+#include <limits.h>
+#include <sys/sysinfo.h>
+#include <stdlib.h>
+
+#define TST_NO_DEFAULT_MAIN
+#include "tst_test.h"
+
+#define BLOCKSIZE (16 * 1024 * 1024)
+
+void tst_pollute_memory(size_t maxsize, int fillchar)
+{
+	size_t i, map_count = 0, safety = 0, blocksize = BLOCKSIZE;
+	void **map_blocks;
+	struct sysinfo info;
+
+	SAFE_SYSINFO(&info);
+	safety = 4096 * SAFE_SYSCONF(_SC_PAGESIZE) / info.mem_unit;
+
+	if (info.freeswap > safety)
+		safety = 0;
+
+	/* Not enough free memory to avoid invoking OOM killer */
+	if (info.freeram <= safety)
+		return;
+
+	if (!maxsize)
+		maxsize = SIZE_MAX;
+
+	if (info.freeram - safety < maxsize / info.mem_unit)
+		maxsize = (info.freeram - safety) * info.mem_unit;
+
+	blocksize = MIN(maxsize, blocksize);
+	map_count = maxsize / blocksize;
+	map_blocks = SAFE_MALLOC(map_count * sizeof(void *));
+
+	/*
+	 * Keep allocating until the first failure. The address space may be
+	 * too fragmented or just smaller than maxsize.
+	 */
+	for (i = 0; i < map_count; i++) {
+		map_blocks[i] = mmap(NULL, blocksize, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+		if (map_blocks[i] == MAP_FAILED) {
+			map_count = i;
+			break;
+		}
+
+		memset(map_blocks[i], fillchar, blocksize);
+	}
+
+	for (i = 0; i < map_count; i++)
+		SAFE_MUNMAP(map_blocks[i], blocksize);
+
+	free(map_blocks);
+}
-- 
2.28.0

[LTP] [PATCH v2 2/4] Add tst_pollute_memory() helper function

[Petr Vorel]

Hi Martin,

> tst_pollute_memory() fills available RAM up to specified limit with given fill
> byte. Useful for testing data disclosure vulnerablities.

Looks nice.
Minor note below.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
> +++ b/lib/tst_memutils.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 SUSE LLC <mdoucha@suse.cz>
> + */
> +
> +#include <unistd.h>
> +#include <limits.h>
> +#include <sys/sysinfo.h>
> +#include <stdlib.h>
> +
> +#define TST_NO_DEFAULT_MAIN
> +#include "tst_test.h"
> +
> +#define BLOCKSIZE (16 * 1024 * 1024)
> +
> +void tst_pollute_memory(size_t maxsize, int fillchar)
> +{
> +	size_t i, map_count = 0, safety = 0, blocksize = BLOCKSIZE;
> +	void **map_blocks;
> +	struct sysinfo info;
> +
> +	SAFE_SYSINFO(&info);
> +	safety = 4096 * SAFE_SYSCONF(_SC_PAGESIZE) / info.mem_unit;
Out of curiosity, how did you figure out safety?
> +
> +	if (info.freeswap > safety)
> +		safety = 0;
> +
> +	/* Not enough free memory to avoid invoking OOM killer */
> +	if (info.freeram <= safety)
Maybe print TINFO / TWARN here?
> +		return;
> +
> +	if (!maxsize)
> +		maxsize = SIZE_MAX;
> +
> +	if (info.freeram - safety < maxsize / info.mem_unit)
> +		maxsize = (info.freeram - safety) * info.mem_unit;
Don't we also want to use info.bufferram ?
> +
> +	blocksize = MIN(maxsize, blocksize);
> +	map_count = maxsize / blocksize;
> +	map_blocks = SAFE_MALLOC(map_count * sizeof(void *));
> +
> +	/*
> +	 * Keep allocating until the first failure. The address space may be
> +	 * too fragmented or just smaller than maxsize.
> +	 */
> +	for (i = 0; i < map_count; i++) {
> +		map_blocks[i] = mmap(NULL, blocksize, PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +
> +		if (map_blocks[i] == MAP_FAILED) {
> +			map_count = i;
> +			break;
> +		}
> +
> +		memset(map_blocks[i], fillchar, blocksize);
> +	}
> +
> +	for (i = 0; i < map_count; i++)
> +		SAFE_MUNMAP(map_blocks[i], blocksize);
> +
> +	free(map_blocks);
> +}

Kind regards,
Petr

[LTP] [PATCH v2 3/4] ioctl_sg01: Pollute free memory in setup

[Martin Doucha]

The test wasn't reliable if most of available memory was full of zeroes.
Pollute free memory to increase the chance of detecting data leak.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1:
- Split patch
- Use tst_pollute_memory() instead of allocating and pre-polluting
  a fixed-size block of memory in setup().

 testcases/kernel/syscalls/ioctl/ioctl_sg01.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl_sg01.c b/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
index daaa96be5..8c9fd0dae 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
@@ -7,9 +7,7 @@
  * CVE-2018-1000204
  *
  * Test ioctl(SG_IO) and check that kernel doesn't leak data. Requires
- * a read-accessible SCSI-compatible device (e.g. SATA disk). Running oom*
- * test program before this one may increase the chance of successfully
- * reproducing the bug.
+ * a read-accessible generic SCSI device (e.g. a DVD drive).
  *
  * Leak fixed in:
  *
@@ -29,8 +27,9 @@
 #include <sys/ioctl.h>
 #include <stdio.h>
 #include "tst_test.h"
+#include "tst_memutils.h"
 
-#define BUF_SIZE 128 * 4096
+#define BUF_SIZE (128 * 4096)
 #define CMD_SIZE 6
 
 static int devfd = -1;
@@ -80,6 +79,10 @@ static void setup(void)
 		tst_brk(TCONF, "Could not find any usable SCSI device");
 
 	tst_res(TINFO, "Found SCSI device %s", devpath);
+
+	/* Pollute some memory to avoid false negatives */
+	tst_pollute_memory(0, 0x42);
+
 	devfd = SAFE_OPEN(devpath, O_RDONLY);
 	query.interface_id = 'S';
 	query.dxfer_direction = SG_DXFER_FROM_DEV;
-- 
2.28.0

[LTP] [PATCH v2 3/4] ioctl_sg01: Pollute free memory in setup

[Petr Vorel]

Hi Martin,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> The test wasn't reliable if most of available memory was full of zeroes.
> Pollute free memory to increase the chance of detecting data leak.

> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---

> Changes since v1:
> - Split patch
> - Use tst_pollute_memory() instead of allocating and pre-polluting
>   a fixed-size block of memory in setup().

>  testcases/kernel/syscalls/ioctl/ioctl_sg01.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_sg01.c b/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
> index daaa96be5..8c9fd0dae 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
> @@ -7,9 +7,7 @@
>   * CVE-2018-1000204
>   *
>   * Test ioctl(SG_IO) and check that kernel doesn't leak data. Requires
> - * a read-accessible SCSI-compatible device (e.g. SATA disk). Running oom*
> - * test program before this one may increase the chance of successfully
> - * reproducing the bug.
> + * a read-accessible generic SCSI device (e.g. a DVD drive).
>   *
>   * Leak fixed in:
>   *
> @@ -29,8 +27,9 @@
>  #include <sys/ioctl.h>
>  #include <stdio.h>
>  #include "tst_test.h"
> +#include "tst_memutils.h"

> -#define BUF_SIZE 128 * 4096
> +#define BUF_SIZE (128 * 4096)
>  #define CMD_SIZE 6

>  static int devfd = -1;
> @@ -80,6 +79,10 @@ static void setup(void)
>  		tst_brk(TCONF, "Could not find any usable SCSI device");

>  	tst_res(TINFO, "Found SCSI device %s", devpath);
> +
> +	/* Pollute some memory to avoid false negatives */
> +	tst_pollute_memory(0, 0x42);
> +
>  	devfd = SAFE_OPEN(devpath, O_RDONLY);
>  	query.interface_id = 'S';
>  	query.dxfer_direction = SG_DXFER_FROM_DEV;

[LTP] [PATCH v2 4/4] ioctl_sg01: Loop data leak check 100 times

[Martin Doucha]

Even with pre-polluted memory, running the test just once might result in
a false negative. Loop it a few times to increase reliability.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1:
- New patch (split)

 testcases/kernel/syscalls/ioctl/ioctl_sg01.c | 21 +++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl_sg01.c b/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
index 8c9fd0dae..8ad2ffed5 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_sg01.c
@@ -100,19 +100,22 @@ static void cleanup(void)
 
 static void run(void)
 {
-	size_t i;
+	size_t i, j;
 
 	memset(buffer, 0, BUF_SIZE);
-	TEST(ioctl(devfd, SG_IO, &query));
 
-	if (TST_RET != 0 && TST_RET != -1)
-		tst_brk(TBROK | TTERRNO, "Invalid ioctl() return value");
+	for (i = 0; i < 100; i++) {
+		TEST(ioctl(devfd, SG_IO, &query));
 
-	/* Check the output buffer even if ioctl() failed, just in case. */
-	for (i = 0; i < BUF_SIZE; i++) {
-		if (buffer[i]) {
-			tst_res(TFAIL, "Kernel memory leaked");
-			return;
+		if (TST_RET != 0 && TST_RET != -1)
+			tst_brk(TBROK|TTERRNO, "Invalid ioctl() return value");
+
+		/* Check the buffer even if ioctl() failed, just in case. */
+		for (j = 0; j < BUF_SIZE; j++) {
+			if (buffer[j]) {
+				tst_res(TFAIL, "Kernel memory leaked");
+				return;
+			}
 		}
 	}
 
-- 
2.28.0

[LTP] [PATCH v2 4/4] ioctl_sg01: Loop data leak check 100 times

[Petr Vorel]

Hi Martin,

> Even with pre-polluted memory, running the test just once might result in
> a false negative. Loop it a few times to increase reliability.

Obviously ok.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

BTW do I understand the test correctly: we expect ioctl() return -1 because we
use uninitialized command[CMD_SIZE] in query.cmdp (as the requirement for empty
command in kernel commit message)?

Kind regards,
Petr

[LTP] [PATCH v2 4/4] ioctl_sg01: Loop data leak check 100 times

[Martin Doucha]

On 02. 09. 20 19:17, Petr Vorel wrote:
> BTW do I understand the test correctly: we expect ioctl() return -1 because we
> use uninitialized command[CMD_SIZE] in query.cmdp (as the requirement for empty
> command in kernel commit message)?

command[CMD_SIZE] is initialized to 0 which is the SCSI command TEST
UNIT READY. We expect ioctl() to return 0 but also ignore -1 because the
only thing we really care about are the contents of query.dxferp buffer.
If ioctl() fails for some legitimate reason but kernel still fills the
buffer with private data, we need to report that the CVE is present.

https://en.wikipedia.org/wiki/SCSI_command

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH v2 4/4] ioctl_sg01: Loop data leak check 100 times

[Petr Vorel]

> On 02. 09. 20 19:17, Petr Vorel wrote:
> > BTW do I understand the test correctly: we expect ioctl() return -1 because we
> > use uninitialized command[CMD_SIZE] in query.cmdp (as the requirement for empty
> > command in kernel commit message)?

> command[CMD_SIZE] is initialized to 0 which is the SCSI command TEST
> UNIT READY. We expect ioctl() to return 0 but also ignore -1 because the
> only thing we really care about are the contents of query.dxferp buffer.
> If ioctl() fails for some legitimate reason but kernel still fills the
> buffer with private data, we need to report that the CVE is present.
Thanks for info, Martin.

Kind regards,
Petr

> https://en.wikipedia.org/wiki/SCSI_command

[LTP] [PATCH v2 0/4] Improve reliability of ioctl_sg01

[Petr Vorel]

Hi Martin,

patchset merged, thanks!

Kind regards,
Petr