[LTP] [PATCH] Add setsockopt08, CVE-2021-22555

[LTP] [PATCH] Add setsockopt08, CVE-2021-22555

[Richard Palethorpe]

This is a copy and paste of Nicolai's reproducer. The main difference
is that I moved some code around. Of course I also used LTP library
features, but essentially it works the same.

There are some hard coded values which I do not like. I guess these
could be calculated or varied somehow. However I struggle to understand
what the kernel is doing. This perhaps needs more investigation. We
could try generalising this test and setsockopt03.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Cc: Nicolai Stange <nstange@suse.de>
---

runtest/cve                                   |   1 +
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/setsockopt/.gitignore     |   1 +
 .../kernel/syscalls/setsockopt/setsockopt08.c | 151 ++++++++++++++++++
 4 files changed, 154 insertions(+)
 create mode 100644 testcases/kernel/syscalls/setsockopt/setsockopt08.c

diff --git a/runtest/cve b/runtest/cve
index 8aa048a40..60bd22f01 100644
--- a/runtest/cve
+++ b/runtest/cve
@@ -65,4 +65,5 @@ cve-2020-14416 pty03
 cve-2020-25705 icmp_rate_limit01
 cve-2020-29373 io_uring02
 cve-2021-3444 bpf_prog05
+cve-2021-22555 setsockopt08 -i 100
 cve-2021-26708 vsock01
diff --git a/runtest/syscalls b/runtest/syscalls
index b379b2d90..2222990fe 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1378,6 +1378,7 @@ setsockopt04 setsockopt04
 setsockopt05 setsockopt05
 setsockopt06 setsockopt06
 setsockopt07 setsockopt07
+setsockopt08 setsockopt08
 
 settimeofday01 settimeofday01
 settimeofday02 settimeofday02
diff --git a/testcases/kernel/syscalls/setsockopt/.gitignore b/testcases/kernel/syscalls/setsockopt/.gitignore
index 1ca5b836b..95a5e43f8 100644
--- a/testcases/kernel/syscalls/setsockopt/.gitignore
+++ b/testcases/kernel/syscalls/setsockopt/.gitignore
@@ -5,3 +5,4 @@
 /setsockopt05
 /setsockopt06
 /setsockopt07
+/setsockopt08
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
new file mode 100644
index 000000000..e77e40aa1
--- /dev/null
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com>
+ * Based on reproducer by Nicolai Stange based on PoC Andy Nguyen
+ */
+/*\
+ * [Description]
+ *
+ * This will reproduce the bug on x86_64 in 32bit compatability
+ * mode. It is most reliable with KASAN enabled. Otherwise it relies
+ * on the out-of-bounds write corrupting something which leads to a
+ * crash. It will run in other scenarious, but is not a test for the
+ * CVE.
+ *
+ * See https://google.github.io/security-research/pocs/linux/cve-2021-22555/writeup.html
+ *
+ * Also below is Nicolai's detailed description of the bug itself.
+ *
+ * The problem underlying CVE-2021-22555 fixed by upstream commit
+ * b29c457a6511 ("netfilter: x_tables: fix compat match/target pad
+ * out-of-bound write") is that the (now removed) padding zeroing code
+ * in xt_compat_target_from_user() had been based on the premise that
+ * the user specified ->u.user.target_size, which will be considered
+ * for the target buffer allocation size, is greater or equal than
+ * what's needed to fit the corresponding xt_target instance's
+ * ->targetsize: if OTOH the user specified ->u.user.target_size is
+ * too small, then the memset() destination address calculated by
+ * adding ->targetsize to the payload start will not point at, but
+ * into or even past the padding. For the table's last entry's target
+ * record, this will result in an out-of-bounds write past the
+ * destination buffer allocated for the converted table. The code
+ * below will create a (compat) table such that the converted table's
+ * calculated size will fit exactly into a slab size of 1024 bytes and
+ * that the memset() in xt_compat_target_from_user() will write past
+ * this slab.
+ *
+ * The table will consist of
+ *  - the mandatory struct compat_ipt_replace header,
+ *  - a single entry consisting of
+ *    - the mandatory compat_ipt_entry header
+ *    - a single 'state' match entry of appropriate size for
+ *      controlling the out-of-bounds write when converting
+ *      the target entry following next,
+ *    - a single 'REJECT' target entry.
+ * The kernel will transform this into a buffer containing (in
+ * this order)
+ * - a xt_table_info
+ * - a single entry consisting of
+ *   - its ipt_entry header
+ *   - a single 'state' match entry
+ *   - followed by a single 'REJECT' target entry.
+ *
+ * The expected sizes for the 'state' match entries as well as the
+ * 'REJECT' target are the size of the base header struct (32 bytes)
+ * plus the size of an unsigned int (4 bytes) each. In the course of
+ * the compat => non-compat conversion, the kernel will insert four
+ * bytes of padding after the unsigned int payload (c.f.  'off'
+ * adjustments via xt_compat_match_offset() and
+ * xt_compat_target_offset() in xt_compat_match_from_user() and
+ * xt_compat_target_from_user() resp.). This code is based on the
+ * premise that the user sets the given ->u.user.match_size or
+ * ->u.user.target_size consistent to the COMPAT_XT_ALIGN()ed payload
+ * size as specified by the corresponding xt_match instance's
+ * ->matchsize or xt_target instance's ->targetsize. That is, the
+ * padding gets inserted unconditionally during the transformation,
+ * independent of the actual values of ->u.user.match_size or
+ * ->u.user.target_size and the result ends up getting layed out with
+ * proper alignment only if said values match the expectations. That's
+ * not a problem in itself, but this unconditional insertion of
+ * padding must be taken into account in the match_size calculation
+ * below.
+ *
+ * For the match_size calculation below, note that the chosen
+ * target slab size is 1024 and that
+ *  - sizeof(xt_table_info) = 64
+ *  - sizeof(ipt_entry) = 112
+ *  - the kernel will insert four bytes of padding
+ *    after the match and target entries each.
+ *  - sizeof(struct xt_entry_target) = 32
+ */
+
+#include "tst_test.h"
+#include "tst_safe_net.h"
+#include "lapi/ip_tables.h"
+#include "lapi/namespaces_constants.h"
+
+static void *buffer;
+
+void setup(void)
+{
+	SAFE_UNSHARE(CLONE_NEWUSER);
+	SAFE_UNSHARE(CLONE_NEWNET);
+}
+
+void run(void)
+{
+	struct ipt_replace *ipt_replace = buffer;
+	struct ipt_entry *ipt_entry = &ipt_replace->entries[0];
+	struct xt_entry_match *xt_entry_match =
+		(struct xt_entry_match *)&ipt_entry->elems[0];
+	const size_t tgt_size = 32;
+	const size_t match_size = 1024 - 64 - 112 - 4 - tgt_size - 4;
+	struct xt_entry_target *xt_entry_tgt =
+		((struct xt_entry_target *) (&ipt_entry->elems[0] + match_size));
+	int fd = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
+
+	xt_entry_match->u.user.match_size = (u_int16_t)match_size;
+	strcpy(xt_entry_match->u.user.name, "state");
+
+	xt_entry_tgt->u.user.target_size = (u_int16_t)tgt_size;
+	strcpy(xt_entry_tgt->u.user.name, "REJECT");
+
+	ipt_entry->target_offset =
+		(__builtin_offsetof(struct ipt_entry, elems) + match_size);
+	ipt_entry->next_offset = ipt_entry->target_offset + tgt_size;
+
+	strcpy(ipt_replace->name, "filter");
+	ipt_replace->num_entries = 1;
+	ipt_replace->num_counters = 1;
+	ipt_replace->size = ipt_entry->next_offset;
+
+	TST_EXP_FAIL(setsockopt(fd, IPPROTO_IP, IPT_SO_SET_REPLACE, buffer, 1),
+		     0,
+		     "setsockopt(%d, IPPROTO_IP, IPT_SO_SET_REPLACE, %p, 1)",
+		     fd, buffer);
+
+	SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.taint_check = TST_TAINT_W | TST_TAINT_D,
+	.forks_child = 1,
+	.bufs = (struct tst_buffers []) {
+		{&buffer, .size = 2048},
+		{},
+	},
+	.needs_kconfigs = (const char *[]) {
+		"CONFIG_NETFILTER_XT_MATCH_STATE",
+		"CONFIG_IP_NF_TARGET_REJECT",
+		"CONFIG_USER_NS=y",
+		"CONFIG_NET_NS=y",
+		NULL
+	},
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "b29c457a6511"},
+		{"CVE", "2021-22555"},
+		{}
+	}
+};
-- 
2.31.1

[LTP] [PATCH v2 1/2] Add lapi/ip_tables.h and use it in setsockopt03

[Richard Palethorpe]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

Oops forgot to add the file below.

V2:
* Add ip_tables.h compatability header

 include/lapi/ip_tables.h                      | 46 +++++++++++++++++++
 .../kernel/syscalls/setsockopt/setsockopt03.c | 39 +---------------
 2 files changed, 48 insertions(+), 37 deletions(-)
 create mode 100644 include/lapi/ip_tables.h

diff --git a/include/lapi/ip_tables.h b/include/lapi/ip_tables.h
new file mode 100644
index 000000000..e91238ffd
--- /dev/null
+++ b/include/lapi/ip_tables.h
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#ifndef LAPI_IP_TABLES__
+#define LAPI_IP_TABLES__
+
+#include "config.h"
+
+#include <linux/netfilter_ipv4/ip_tables.h>
+
+#ifndef HAVE_STRUCT_XT_ENTRY_MATCH
+struct xt_entry_match {
+	union {
+		struct {
+			uint16_t match_size;
+			char name[29];
+			uint8_t revision;
+		} user;
+		struct {
+			uint16_t match_size;
+			void *match;
+		} kernel;
+		uint16_t match_size;
+	} u;
+	unsigned char data[0];
+};
+#endif
+
+#ifndef HAVE_STRUCT_XT_ENTRY_TARGET
+struct xt_entry_target {
+	union {
+		struct {
+			uint16_t target_size;
+			char name[29];
+			uint8_t revision;
+		} user;
+		struct {
+			uint16_t target_size;
+			void *target;
+		} kernel;
+		uint16_t target_size;
+	} u;
+	unsigned char data[0];
+};
+#endif
+
+#endif
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt03.c b/testcases/kernel/syscalls/setsockopt/setsockopt03.c
index 3d49628d6..1434475db 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt03.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt03.c
@@ -21,12 +21,13 @@
 #include <netinet/in.h>
 #include <net/if.h>
 #include <limits.h>
-#include <linux/netfilter_ipv4/ip_tables.h>
 
 #include "tst_test.h"
 #include "tst_safe_net.h"
 #include "tst_kernel.h"
 
+#include "lapi/ip_tables.h"
+
 #define TOO_SMALL_OFFSET 74
 #define OFFSET_OVERWRITE 0xFFFF
 #define NEXT_OFFSET (sizeof(struct ipt_entry)		\
@@ -34,42 +35,6 @@
 		     + sizeof(struct xt_entry_target))
 #define PADDING (OFFSET_OVERWRITE - NEXT_OFFSET)
 
-#ifndef HAVE_STRUCT_XT_ENTRY_MATCH
-struct xt_entry_match {
-	union {
-		struct {
-			uint16_t match_size;
-			char name[29];
-			uint8_t revision;
-		} user;
-		struct {
-			uint16_t match_size;
-			void *match;
-		} kernel;
-		uint16_t match_size;
-	} u;
-	unsigned char data[0];
-};
-#endif
-
-#ifndef HAVE_STRUCT_XT_ENTRY_TARGET
-struct xt_entry_target {
-	union {
-		struct {
-			uint16_t target_size;
-			char name[29];
-			uint8_t revision;
-		} user;
-		struct {
-			uint16_t target_size;
-			void *target;
-		} kernel;
-		uint16_t target_size;
-	} u;
-	unsigned char data[0];
-};
-#endif
-
 struct payload {
 	struct ipt_replace repl;
 	struct ipt_entry ent;
-- 
2.31.1

[LTP] [PATCH v2 2/2] Add setsockopt08, CVE-2021-22555

[Richard Palethorpe]

This is a copy and paste of Nicolai's reproducer. The main difference
is that I moved some code around. Of course I also used LTP library
features, but essentially it works the same.

There are some hard coded values which I do not like. I guess these
could be calculated or varied somehow. However I struggle to understand
what the kernel is doing. This perhaps needs more investigation. We
could try generalising this test and setsockopt03.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Cc: Nicolai Stange <nstange@suse.de>
---
 runtest/cve                                   |   1 +
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/setsockopt/.gitignore     |   1 +
 .../kernel/syscalls/setsockopt/setsockopt08.c | 151 ++++++++++++++++++
 4 files changed, 154 insertions(+)
 create mode 100644 testcases/kernel/syscalls/setsockopt/setsockopt08.c

diff --git a/runtest/cve b/runtest/cve
index 8aa048a40..60bd22f01 100644
--- a/runtest/cve
+++ b/runtest/cve
@@ -65,4 +65,5 @@ cve-2020-14416 pty03
 cve-2020-25705 icmp_rate_limit01
 cve-2020-29373 io_uring02
 cve-2021-3444 bpf_prog05
+cve-2021-22555 setsockopt08 -i 100
 cve-2021-26708 vsock01
diff --git a/runtest/syscalls b/runtest/syscalls
index b379b2d90..2222990fe 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1378,6 +1378,7 @@ setsockopt04 setsockopt04
 setsockopt05 setsockopt05
 setsockopt06 setsockopt06
 setsockopt07 setsockopt07
+setsockopt08 setsockopt08
 
 settimeofday01 settimeofday01
 settimeofday02 settimeofday02
diff --git a/testcases/kernel/syscalls/setsockopt/.gitignore b/testcases/kernel/syscalls/setsockopt/.gitignore
index 1ca5b836b..95a5e43f8 100644
--- a/testcases/kernel/syscalls/setsockopt/.gitignore
+++ b/testcases/kernel/syscalls/setsockopt/.gitignore
@@ -5,3 +5,4 @@
 /setsockopt05
 /setsockopt06
 /setsockopt07
+/setsockopt08
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
new file mode 100644
index 000000000..e77e40aa1
--- /dev/null
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com>
+ * Based on reproducer by Nicolai Stange based on PoC Andy Nguyen
+ */
+/*\
+ * [Description]
+ *
+ * This will reproduce the bug on x86_64 in 32bit compatability
+ * mode. It is most reliable with KASAN enabled. Otherwise it relies
+ * on the out-of-bounds write corrupting something which leads to a
+ * crash. It will run in other scenarious, but is not a test for the
+ * CVE.
+ *
+ * See https://google.github.io/security-research/pocs/linux/cve-2021-22555/writeup.html
+ *
+ * Also below is Nicolai's detailed description of the bug itself.
+ *
+ * The problem underlying CVE-2021-22555 fixed by upstream commit
+ * b29c457a6511 ("netfilter: x_tables: fix compat match/target pad
+ * out-of-bound write") is that the (now removed) padding zeroing code
+ * in xt_compat_target_from_user() had been based on the premise that
+ * the user specified ->u.user.target_size, which will be considered
+ * for the target buffer allocation size, is greater or equal than
+ * what's needed to fit the corresponding xt_target instance's
+ * ->targetsize: if OTOH the user specified ->u.user.target_size is
+ * too small, then the memset() destination address calculated by
+ * adding ->targetsize to the payload start will not point at, but
+ * into or even past the padding. For the table's last entry's target
+ * record, this will result in an out-of-bounds write past the
+ * destination buffer allocated for the converted table. The code
+ * below will create a (compat) table such that the converted table's
+ * calculated size will fit exactly into a slab size of 1024 bytes and
+ * that the memset() in xt_compat_target_from_user() will write past
+ * this slab.
+ *
+ * The table will consist of
+ *  - the mandatory struct compat_ipt_replace header,
+ *  - a single entry consisting of
+ *    - the mandatory compat_ipt_entry header
+ *    - a single 'state' match entry of appropriate size for
+ *      controlling the out-of-bounds write when converting
+ *      the target entry following next,
+ *    - a single 'REJECT' target entry.
+ * The kernel will transform this into a buffer containing (in
+ * this order)
+ * - a xt_table_info
+ * - a single entry consisting of
+ *   - its ipt_entry header
+ *   - a single 'state' match entry
+ *   - followed by a single 'REJECT' target entry.
+ *
+ * The expected sizes for the 'state' match entries as well as the
+ * 'REJECT' target are the size of the base header struct (32 bytes)
+ * plus the size of an unsigned int (4 bytes) each. In the course of
+ * the compat => non-compat conversion, the kernel will insert four
+ * bytes of padding after the unsigned int payload (c.f.  'off'
+ * adjustments via xt_compat_match_offset() and
+ * xt_compat_target_offset() in xt_compat_match_from_user() and
+ * xt_compat_target_from_user() resp.). This code is based on the
+ * premise that the user sets the given ->u.user.match_size or
+ * ->u.user.target_size consistent to the COMPAT_XT_ALIGN()ed payload
+ * size as specified by the corresponding xt_match instance's
+ * ->matchsize or xt_target instance's ->targetsize. That is, the
+ * padding gets inserted unconditionally during the transformation,
+ * independent of the actual values of ->u.user.match_size or
+ * ->u.user.target_size and the result ends up getting layed out with
+ * proper alignment only if said values match the expectations. That's
+ * not a problem in itself, but this unconditional insertion of
+ * padding must be taken into account in the match_size calculation
+ * below.
+ *
+ * For the match_size calculation below, note that the chosen
+ * target slab size is 1024 and that
+ *  - sizeof(xt_table_info) = 64
+ *  - sizeof(ipt_entry) = 112
+ *  - the kernel will insert four bytes of padding
+ *    after the match and target entries each.
+ *  - sizeof(struct xt_entry_target) = 32
+ */
+
+#include "tst_test.h"
+#include "tst_safe_net.h"
+#include "lapi/ip_tables.h"
+#include "lapi/namespaces_constants.h"
+
+static void *buffer;
+
+void setup(void)
+{
+	SAFE_UNSHARE(CLONE_NEWUSER);
+	SAFE_UNSHARE(CLONE_NEWNET);
+}
+
+void run(void)
+{
+	struct ipt_replace *ipt_replace = buffer;
+	struct ipt_entry *ipt_entry = &ipt_replace->entries[0];
+	struct xt_entry_match *xt_entry_match =
+		(struct xt_entry_match *)&ipt_entry->elems[0];
+	const size_t tgt_size = 32;
+	const size_t match_size = 1024 - 64 - 112 - 4 - tgt_size - 4;
+	struct xt_entry_target *xt_entry_tgt =
+		((struct xt_entry_target *) (&ipt_entry->elems[0] + match_size));
+	int fd = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
+
+	xt_entry_match->u.user.match_size = (u_int16_t)match_size;
+	strcpy(xt_entry_match->u.user.name, "state");
+
+	xt_entry_tgt->u.user.target_size = (u_int16_t)tgt_size;
+	strcpy(xt_entry_tgt->u.user.name, "REJECT");
+
+	ipt_entry->target_offset =
+		(__builtin_offsetof(struct ipt_entry, elems) + match_size);
+	ipt_entry->next_offset = ipt_entry->target_offset + tgt_size;
+
+	strcpy(ipt_replace->name, "filter");
+	ipt_replace->num_entries = 1;
+	ipt_replace->num_counters = 1;
+	ipt_replace->size = ipt_entry->next_offset;
+
+	TST_EXP_FAIL(setsockopt(fd, IPPROTO_IP, IPT_SO_SET_REPLACE, buffer, 1),
+		     0,
+		     "setsockopt(%d, IPPROTO_IP, IPT_SO_SET_REPLACE, %p, 1)",
+		     fd, buffer);
+
+	SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.taint_check = TST_TAINT_W | TST_TAINT_D,
+	.forks_child = 1,
+	.bufs = (struct tst_buffers []) {
+		{&buffer, .size = 2048},
+		{},
+	},
+	.needs_kconfigs = (const char *[]) {
+		"CONFIG_NETFILTER_XT_MATCH_STATE",
+		"CONFIG_IP_NF_TARGET_REJECT",
+		"CONFIG_USER_NS=y",
+		"CONFIG_NET_NS=y",
+		NULL
+	},
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "b29c457a6511"},
+		{"CVE", "2021-22555"},
+		{}
+	}
+};
-- 
2.31.1

[LTP] [PATCH v2 2/2] Add setsockopt08, CVE-2021-22555

[Martin Doucha]

Hi,
since this vulnerability is in the compat syscall wrappers, you should
also copy the arch bits check from setsockopt03 setup().

On 03. 08. 21 10:38, Richard Palethorpe via ltp wrote:
> This is a copy and paste of Nicolai's reproducer. The main difference
> is that I moved some code around. Of course I also used LTP library
> features, but essentially it works the same.
> 
> There are some hard coded values which I do not like. I guess these
> could be calculated or varied somehow. However I struggle to understand
> what the kernel is doing. This perhaps needs more investigation. We
> could try generalising this test and setsockopt03

-- 
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 2/2] Add setsockopt08, CVE-2021-22555

[Richard Palethorpe]

Hi,

Martin Doucha <mdoucha@suse.cz> writes:

> Hi,
> since this vulnerability is in the compat syscall wrappers, you should
> also copy the arch bits check from setsockopt03 setup().

+1

Will roll another patch after a delay.

>
> On 03. 08. 21 10:38, Richard Palethorpe via ltp wrote:
>> This is a copy and paste of Nicolai's reproducer. The main difference
>> is that I moved some code around. Of course I also used LTP library
>> features, but essentially it works the same.
>> 
>> There are some hard coded values which I do not like. I guess these
>> could be calculated or varied somehow. However I struggle to understand
>> what the kernel is doing. This perhaps needs more investigation. We
>> could try generalising this test and setsockopt03


-- 
Thank you,
Richard.

[LTP] [PATCH v3 1/3] API: TST_EXP_FAIL: Allow passing when errno is not checked

[Richard Palethorpe]

Judging by the existing usage of TST_EXP_FAIL(..., 0, ...) in
finit_module02. We want to pass if errno == 0 otherwise the test will
not return a result.

This is also less surprising than giving errno == 0 a dual
meaning.

This also changes the trailing '\' indentation to tabs. However this
is correct and the rest of the file is wrong.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V3:
* Add fix for TST_EXP_FAIL which prevented the test from
  passing on a non buggy system.
* TCONF but continue on non 32-bit compat mode
* Add Fixes trailer

V2:
* Add mising lapi header

 include/tst_test_macros.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/include/tst_test_macros.h b/include/tst_test_macros.h
index 41886fbbc..4c1df58ff 100644
--- a/include/tst_test_macros.h
+++ b/include/tst_test_macros.h
@@ -137,17 +137,15 @@ extern void *TST_RET_PTR;
 			break;                                                 \
 		}                                                              \
 		                                                               \
-		if (ERRNO) {                                                   \
-			if (TST_ERR == ERRNO) {                                \
-				TST_MSG_(TPASS | TTERRNO, " ",                 \
-				         #SCALL, ##__VA_ARGS__);               \
-				TST_PASS = 1;                                  \
-			} else {                                               \
-				TST_MSGP_(TFAIL | TTERRNO, " expected %s",     \
-				          tst_strerrno(ERRNO),                 \
-				          #SCALL, ##__VA_ARGS__);              \
-			}                                                      \
-		}                                                              \
+		if (!ERRNO || TST_ERR == ERRNO) {			\
+			TST_MSG_(TPASS | TTERRNO, " ",			\
+				 #SCALL, ##__VA_ARGS__);		\
+			TST_PASS = 1;					\
+		} else {						\
+			TST_MSGP_(TFAIL | TTERRNO, " expected %s",	\
+				  tst_strerrno(ERRNO),			\
+				  #SCALL, ##__VA_ARGS__);		\
+		}							\
 	} while (0)
 
 #define TST_EXP_FAIL(SCALL, ERRNO, ...) TST_EXP_FAIL_(TST_RET == 0, SCALL, ERRNO, __VA_ARGS__)
-- 
2.31.1

[LTP] [PATCH v3 2/3] API: Add lapi/ip_tables.h and use it in setsockopt03

[Richard Palethorpe]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/lapi/ip_tables.h                      | 46 +++++++++++++++++++
 .../kernel/syscalls/setsockopt/setsockopt03.c | 39 +---------------
 2 files changed, 48 insertions(+), 37 deletions(-)
 create mode 100644 include/lapi/ip_tables.h

diff --git a/include/lapi/ip_tables.h b/include/lapi/ip_tables.h
new file mode 100644
index 000000000..e91238ffd
--- /dev/null
+++ b/include/lapi/ip_tables.h
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#ifndef LAPI_IP_TABLES__
+#define LAPI_IP_TABLES__
+
+#include "config.h"
+
+#include <linux/netfilter_ipv4/ip_tables.h>
+
+#ifndef HAVE_STRUCT_XT_ENTRY_MATCH
+struct xt_entry_match {
+	union {
+		struct {
+			uint16_t match_size;
+			char name[29];
+			uint8_t revision;
+		} user;
+		struct {
+			uint16_t match_size;
+			void *match;
+		} kernel;
+		uint16_t match_size;
+	} u;
+	unsigned char data[0];
+};
+#endif
+
+#ifndef HAVE_STRUCT_XT_ENTRY_TARGET
+struct xt_entry_target {
+	union {
+		struct {
+			uint16_t target_size;
+			char name[29];
+			uint8_t revision;
+		} user;
+		struct {
+			uint16_t target_size;
+			void *target;
+		} kernel;
+		uint16_t target_size;
+	} u;
+	unsigned char data[0];
+};
+#endif
+
+#endif
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt03.c b/testcases/kernel/syscalls/setsockopt/setsockopt03.c
index 3d49628d6..1434475db 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt03.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt03.c
@@ -21,12 +21,13 @@
 #include <netinet/in.h>
 #include <net/if.h>
 #include <limits.h>
-#include <linux/netfilter_ipv4/ip_tables.h>
 
 #include "tst_test.h"
 #include "tst_safe_net.h"
 #include "tst_kernel.h"
 
+#include "lapi/ip_tables.h"
+
 #define TOO_SMALL_OFFSET 74
 #define OFFSET_OVERWRITE 0xFFFF
 #define NEXT_OFFSET (sizeof(struct ipt_entry)		\
@@ -34,42 +35,6 @@
 		     + sizeof(struct xt_entry_target))
 #define PADDING (OFFSET_OVERWRITE - NEXT_OFFSET)
 
-#ifndef HAVE_STRUCT_XT_ENTRY_MATCH
-struct xt_entry_match {
-	union {
-		struct {
-			uint16_t match_size;
-			char name[29];
-			uint8_t revision;
-		} user;
-		struct {
-			uint16_t match_size;
-			void *match;
-		} kernel;
-		uint16_t match_size;
-	} u;
-	unsigned char data[0];
-};
-#endif
-
-#ifndef HAVE_STRUCT_XT_ENTRY_TARGET
-struct xt_entry_target {
-	union {
-		struct {
-			uint16_t target_size;
-			char name[29];
-			uint8_t revision;
-		} user;
-		struct {
-			uint16_t target_size;
-			void *target;
-		} kernel;
-		uint16_t target_size;
-	} u;
-	unsigned char data[0];
-};
-#endif
-
 struct payload {
 	struct ipt_replace repl;
 	struct ipt_entry ent;
-- 
2.31.1

[LTP] [PATCH v3 3/3] Add setsockopt08, CVE-2021-22555

[Richard Palethorpe]

This is a copy and paste of Nicolai's reproducer. The main difference
is that I moved some code around. Of course I also used LTP library
features, but essentially it works the same.

There are some hard coded values which I do not like. I guess these
could be calculated or varied somehow. However I struggle to understand
what the kernel is doing. This perhaps needs more investigation. We
could try generalising this test and setsockopt03.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Fixes: #850
---
 runtest/cve                                   |   1 +
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/setsockopt/.gitignore     |   1 +
 .../kernel/syscalls/setsockopt/setsockopt08.c | 156 ++++++++++++++++++
 4 files changed, 159 insertions(+)
 create mode 100644 testcases/kernel/syscalls/setsockopt/setsockopt08.c

diff --git a/runtest/cve b/runtest/cve
index 8aa048a40..60bd22f01 100644
--- a/runtest/cve
+++ b/runtest/cve
@@ -65,4 +65,5 @@ cve-2020-14416 pty03
 cve-2020-25705 icmp_rate_limit01
 cve-2020-29373 io_uring02
 cve-2021-3444 bpf_prog05
+cve-2021-22555 setsockopt08 -i 100
 cve-2021-26708 vsock01
diff --git a/runtest/syscalls b/runtest/syscalls
index b379b2d90..2222990fe 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1378,6 +1378,7 @@ setsockopt04 setsockopt04
 setsockopt05 setsockopt05
 setsockopt06 setsockopt06
 setsockopt07 setsockopt07
+setsockopt08 setsockopt08
 
 settimeofday01 settimeofday01
 settimeofday02 settimeofday02
diff --git a/testcases/kernel/syscalls/setsockopt/.gitignore b/testcases/kernel/syscalls/setsockopt/.gitignore
index 1ca5b836b..95a5e43f8 100644
--- a/testcases/kernel/syscalls/setsockopt/.gitignore
+++ b/testcases/kernel/syscalls/setsockopt/.gitignore
@@ -5,3 +5,4 @@
 /setsockopt05
 /setsockopt06
 /setsockopt07
+/setsockopt08
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
new file mode 100644
index 000000000..3513227a9
--- /dev/null
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com>
+ * Based on reproducer by Nicolai Stange based on PoC Andy Nguyen
+ */
+/*\
+ * [Description]
+ *
+ * This will reproduce the bug on x86_64 in 32bit compatibility
+ * mode. It is most reliable with KASAN enabled. Otherwise it relies
+ * on the out-of-bounds write corrupting something which leads to a
+ * crash. It will run in other scenarious, but is not a test for the
+ * CVE.
+ *
+ * See https://google.github.io/security-research/pocs/linux/cve-2021-22555/writeup.html
+ *
+ * Also below is Nicolai's detailed description of the bug itself.
+ *
+ * The problem underlying CVE-2021-22555 fixed by upstream commit
+ * b29c457a6511 ("netfilter: x_tables: fix compat match/target pad
+ * out-of-bound write") is that the (now removed) padding zeroing code
+ * in xt_compat_target_from_user() had been based on the premise that
+ * the user specified ->u.user.target_size, which will be considered
+ * for the target buffer allocation size, is greater or equal than
+ * what's needed to fit the corresponding xt_target instance's
+ * ->targetsize: if OTOH the user specified ->u.user.target_size is
+ * too small, then the memset() destination address calculated by
+ * adding ->targetsize to the payload start will not point at, but
+ * into or even past the padding. For the table's last entry's target
+ * record, this will result in an out-of-bounds write past the
+ * destination buffer allocated for the converted table. The code
+ * below will create a (compat) table such that the converted table's
+ * calculated size will fit exactly into a slab size of 1024 bytes and
+ * that the memset() in xt_compat_target_from_user() will write past
+ * this slab.
+ *
+ * The table will consist of
+ *  - the mandatory struct compat_ipt_replace header,
+ *  - a single entry consisting of
+ *    - the mandatory compat_ipt_entry header
+ *    - a single 'state' match entry of appropriate size for
+ *      controlling the out-of-bounds write when converting
+ *      the target entry following next,
+ *    - a single 'REJECT' target entry.
+ * The kernel will transform this into a buffer containing (in
+ * this order)
+ * - a xt_table_info
+ * - a single entry consisting of
+ *   - its ipt_entry header
+ *   - a single 'state' match entry
+ *   - followed by a single 'REJECT' target entry.
+ *
+ * The expected sizes for the 'state' match entries as well as the
+ * 'REJECT' target are the size of the base header struct (32 bytes)
+ * plus the size of an unsigned int (4 bytes) each. In the course of
+ * the compat => non-compat conversion, the kernel will insert four
+ * bytes of padding after the unsigned int payload (c.f.  'off'
+ * adjustments via xt_compat_match_offset() and
+ * xt_compat_target_offset() in xt_compat_match_from_user() and
+ * xt_compat_target_from_user() resp.). This code is based on the
+ * premise that the user sets the given ->u.user.match_size or
+ * ->u.user.target_size consistent to the COMPAT_XT_ALIGN()ed payload
+ * size as specified by the corresponding xt_match instance's
+ * ->matchsize or xt_target instance's ->targetsize. That is, the
+ * padding gets inserted unconditionally during the transformation,
+ * independent of the actual values of ->u.user.match_size or
+ * ->u.user.target_size and the result ends up getting layed out with
+ * proper alignment only if said values match the expectations. That's
+ * not a problem in itself, but this unconditional insertion of
+ * padding must be taken into account in the match_size calculation
+ * below.
+ *
+ * For the match_size calculation below, note that the chosen
+ * target slab size is 1024 and that
+ *  - sizeof(xt_table_info) = 64
+ *  - sizeof(ipt_entry) = 112
+ *  - the kernel will insert four bytes of padding
+ *    after the match and target entries each.
+ *  - sizeof(struct xt_entry_target) = 32
+ */
+
+#include "tst_test.h"
+#include "tst_safe_net.h"
+#include "lapi/ip_tables.h"
+#include "lapi/namespaces_constants.h"
+
+static void *buffer;
+
+void setup(void)
+{
+	if (tst_kernel_bits() == 32 || sizeof(long) > 4) {
+		tst_res(TCONF,
+			"The vulnerability was only present in 32-bit compat mode");
+	}
+
+	SAFE_UNSHARE(CLONE_NEWUSER);
+	SAFE_UNSHARE(CLONE_NEWNET);
+}
+
+void run(void)
+{
+	struct ipt_replace *ipt_replace = buffer;
+	struct ipt_entry *ipt_entry = &ipt_replace->entries[0];
+	struct xt_entry_match *xt_entry_match =
+		(struct xt_entry_match *)&ipt_entry->elems[0];
+	const size_t tgt_size = 32;
+	const size_t match_size = 1024 - 64 - 112 - 4 - tgt_size - 4;
+	struct xt_entry_target *xt_entry_tgt =
+		((struct xt_entry_target *) (&ipt_entry->elems[0] + match_size));
+	int fd = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
+
+	xt_entry_match->u.user.match_size = (u_int16_t)match_size;
+	strcpy(xt_entry_match->u.user.name, "state");
+
+	xt_entry_tgt->u.user.target_size = (u_int16_t)tgt_size;
+	strcpy(xt_entry_tgt->u.user.name, "REJECT");
+
+	ipt_entry->target_offset =
+		(__builtin_offsetof(struct ipt_entry, elems) + match_size);
+	ipt_entry->next_offset = ipt_entry->target_offset + tgt_size;
+
+	strcpy(ipt_replace->name, "filter");
+	ipt_replace->num_entries = 1;
+	ipt_replace->num_counters = 1;
+	ipt_replace->size = ipt_entry->next_offset;
+
+	TST_EXP_FAIL(setsockopt(fd, IPPROTO_IP, IPT_SO_SET_REPLACE, buffer, 1),
+		     0,
+		     "setsockopt(%d, IPPROTO_IP, IPT_SO_SET_REPLACE, %p, 1)",
+		     fd, buffer);
+
+	SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.taint_check = TST_TAINT_W | TST_TAINT_D,
+	.forks_child = 1,
+	.bufs = (struct tst_buffers []) {
+		{&buffer, .size = 2048},
+		{},
+	},
+	.needs_kconfigs = (const char *[]) {
+		"CONFIG_NETFILTER_XT_MATCH_STATE",
+		"CONFIG_IP_NF_TARGET_REJECT",
+		"CONFIG_USER_NS=y",
+		"CONFIG_NET_NS=y",
+		NULL
+	},
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "b29c457a6511"},
+		{"CVE", "2021-22555"},
+		{}
+	}
+};
-- 
2.31.1

[LTP] [PATCH v3 1/3] API: TST_EXP_FAIL: Allow passing when errno is not checked

[Cyril Hrubis]

Hi!
> Judging by the existing usage of TST_EXP_FAIL(..., 0, ...) in
> finit_module02. We want to pass if errno == 0 otherwise the test will
> not return a result.

This is actually not true, we do not pass 0 to TST_EXP_FAIL() in
finit_module() at all. The places that are not initialized in the
structure are initialized at runtime based on the kernel version. I do
not think that we even pass 0 to TST_EXP_FAIL*() as an errno anywhere,
but I haven't really checked all the callers.

> This is also less surprising than giving errno == 0 a dual
> meaning.

But I do agree that the current if (ERRNO) branch is confusing. I would
be for dropping the if (ERRNO) and checking the TST_ERR against ERRNO
unconditionally.

Also note that the TEST() macro clears errno, so if a syscall fails but
does not report any error TST_ERR will end up 0 either way so there is
no need for having special handling for 0.

> This also changes the trailing '\' indentation to tabs. However this
> is correct and the rest of the file is wrong.
> 
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
> 
> V3:
> * Add fix for TST_EXP_FAIL which prevented the test from
>   passing on a non buggy system.
> * TCONF but continue on non 32-bit compat mode
> * Add Fixes trailer
> 
> V2:
> * Add mising lapi header
> 
>  include/tst_test_macros.h | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/include/tst_test_macros.h b/include/tst_test_macros.h
> index 41886fbbc..4c1df58ff 100644
> --- a/include/tst_test_macros.h
> +++ b/include/tst_test_macros.h
> @@ -137,17 +137,15 @@ extern void *TST_RET_PTR;
>  			break;                                                 \
>  		}                                                              \
>  		                                                               \
> -		if (ERRNO) {                                                   \
> -			if (TST_ERR == ERRNO) {                                \
> -				TST_MSG_(TPASS | TTERRNO, " ",                 \
> -				         #SCALL, ##__VA_ARGS__);               \
> -				TST_PASS = 1;                                  \
> -			} else {                                               \
> -				TST_MSGP_(TFAIL | TTERRNO, " expected %s",     \
> -				          tst_strerrno(ERRNO),                 \
> -				          #SCALL, ##__VA_ARGS__);              \
> -			}                                                      \
> -		}                                                              \
> +		if (!ERRNO || TST_ERR == ERRNO) {			\
> +			TST_MSG_(TPASS | TTERRNO, " ",			\
> +				 #SCALL, ##__VA_ARGS__);		\
> +			TST_PASS = 1;					\
> +		} else {						\
> +			TST_MSGP_(TFAIL | TTERRNO, " expected %s",	\
> +				  tst_strerrno(ERRNO),			\
> +				  #SCALL, ##__VA_ARGS__);		\
> +		}							\
>  	} while (0)
>  
>  #define TST_EXP_FAIL(SCALL, ERRNO, ...) TST_EXP_FAIL_(TST_RET == 0, SCALL, ERRNO, __VA_ARGS__)
> -- 
> 2.31.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/3] API: TST_EXP_FAIL: Allow passing when errno is not checked

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> Judging by the existing usage of TST_EXP_FAIL(..., 0, ...) in
>> finit_module02. We want to pass if errno == 0 otherwise the test will
>> not return a result.
>
> This is actually not true, we do not pass 0 to TST_EXP_FAIL() in
> finit_module() at all. The places that are not initialized in the
> structure are initialized at runtime based on the kernel version. I do
> not think that we even pass 0 to TST_EXP_FAIL*() as an errno anywhere,
> but I haven't really checked all the callers.
>
>> This is also less surprising than giving errno == 0 a dual
>> meaning.
>
> But I do agree that the current if (ERRNO) branch is confusing. I would
> be for dropping the if (ERRNO) and checking the TST_ERR against ERRNO
> unconditionally.
>
> Also note that the TEST() macro clears errno, so if a syscall fails but
> does not report any error TST_ERR will end up 0 either way so there is
> no need for having special handling for 0.

There is if the errno is set, but is undefined. Like if the resulting
errno is platform or config dependent.

In the present case though we can just check for EINVAL. That is what
the setsockopt man page indicates. Initially I wasn't sure if EFAULT
were equally valid, but it seems not.

I suppose we can fall back to using TEST() if the other case arises.

-- 
Thank you,
Richard.

[LTP] [PATCH v4 1/3] API: TST_EXP_FAIL: Allow passing when errno is not set

[Richard Palethorpe]

TEST() sets errno to zero. So if something does not set errno on
failure it will be zero. It is normal for functions not to use errno,
some standard libraries are the exception.

The original code will not allow for a pass result if errno is not
set. So we can just remove the branch to allow that. It's not clear
what the original intention was.

This also changes the trailing '\' indentation to tabs. However this
is correct and the rest of the file is wrong.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V4:
* Check for EINVAL.
* Unconditionally check TST_ERR == ERRNO.
  (redundant/irrelevant due to the above change, but we still should do it IMO)

V3:
* Add fix for TST_EXP_FAIL which prevented the test from
  passing on a non buggy system.
* TCONF but continue on non 32-bit compat mode
* Add Fixes trailer

V2:
* Add mising lapi header

 include/tst_test_macros.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/include/tst_test_macros.h b/include/tst_test_macros.h
index 41886fbbc..cd65d8d0d 100644
--- a/include/tst_test_macros.h
+++ b/include/tst_test_macros.h
@@ -137,17 +137,15 @@ extern void *TST_RET_PTR;
 			break;                                                 \
 		}                                                              \
 		                                                               \
-		if (ERRNO) {                                                   \
-			if (TST_ERR == ERRNO) {                                \
-				TST_MSG_(TPASS | TTERRNO, " ",                 \
-				         #SCALL, ##__VA_ARGS__);               \
-				TST_PASS = 1;                                  \
-			} else {                                               \
-				TST_MSGP_(TFAIL | TTERRNO, " expected %s",     \
-				          tst_strerrno(ERRNO),                 \
-				          #SCALL, ##__VA_ARGS__);              \
-			}                                                      \
-		}                                                              \
+		if (TST_ERR == ERRNO) {					\
+			TST_MSG_(TPASS | TTERRNO, " ",			\
+				 #SCALL, ##__VA_ARGS__);		\
+			TST_PASS = 1;					\
+		} else {						\
+			TST_MSGP_(TFAIL | TTERRNO, " expected %s",	\
+				  tst_strerrno(ERRNO),			\
+				  #SCALL, ##__VA_ARGS__);		\
+		}							\
 	} while (0)
 
 #define TST_EXP_FAIL(SCALL, ERRNO, ...) TST_EXP_FAIL_(TST_RET == 0, SCALL, ERRNO, __VA_ARGS__)
-- 
2.31.1

[LTP] [PATCH v4 2/3] API: Add lapi/ip_tables.h and use it in setsockopt03

[Richard Palethorpe]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/lapi/ip_tables.h                      | 46 +++++++++++++++++++
 .../kernel/syscalls/setsockopt/setsockopt03.c | 39 +---------------
 2 files changed, 48 insertions(+), 37 deletions(-)
 create mode 100644 include/lapi/ip_tables.h

diff --git a/include/lapi/ip_tables.h b/include/lapi/ip_tables.h
new file mode 100644
index 000000000..e91238ffd
--- /dev/null
+++ b/include/lapi/ip_tables.h
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#ifndef LAPI_IP_TABLES__
+#define LAPI_IP_TABLES__
+
+#include "config.h"
+
+#include <linux/netfilter_ipv4/ip_tables.h>
+
+#ifndef HAVE_STRUCT_XT_ENTRY_MATCH
+struct xt_entry_match {
+	union {
+		struct {
+			uint16_t match_size;
+			char name[29];
+			uint8_t revision;
+		} user;
+		struct {
+			uint16_t match_size;
+			void *match;
+		} kernel;
+		uint16_t match_size;
+	} u;
+	unsigned char data[0];
+};
+#endif
+
+#ifndef HAVE_STRUCT_XT_ENTRY_TARGET
+struct xt_entry_target {
+	union {
+		struct {
+			uint16_t target_size;
+			char name[29];
+			uint8_t revision;
+		} user;
+		struct {
+			uint16_t target_size;
+			void *target;
+		} kernel;
+		uint16_t target_size;
+	} u;
+	unsigned char data[0];
+};
+#endif
+
+#endif
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt03.c b/testcases/kernel/syscalls/setsockopt/setsockopt03.c
index 3d49628d6..1434475db 100644
--- a/testcases/kernel/syscalls/setsockopt/setsockopt03.c
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt03.c
@@ -21,12 +21,13 @@
 #include <netinet/in.h>
 #include <net/if.h>
 #include <limits.h>
-#include <linux/netfilter_ipv4/ip_tables.h>
 
 #include "tst_test.h"
 #include "tst_safe_net.h"
 #include "tst_kernel.h"
 
+#include "lapi/ip_tables.h"
+
 #define TOO_SMALL_OFFSET 74
 #define OFFSET_OVERWRITE 0xFFFF
 #define NEXT_OFFSET (sizeof(struct ipt_entry)		\
@@ -34,42 +35,6 @@
 		     + sizeof(struct xt_entry_target))
 #define PADDING (OFFSET_OVERWRITE - NEXT_OFFSET)
 
-#ifndef HAVE_STRUCT_XT_ENTRY_MATCH
-struct xt_entry_match {
-	union {
-		struct {
-			uint16_t match_size;
-			char name[29];
-			uint8_t revision;
-		} user;
-		struct {
-			uint16_t match_size;
-			void *match;
-		} kernel;
-		uint16_t match_size;
-	} u;
-	unsigned char data[0];
-};
-#endif
-
-#ifndef HAVE_STRUCT_XT_ENTRY_TARGET
-struct xt_entry_target {
-	union {
-		struct {
-			uint16_t target_size;
-			char name[29];
-			uint8_t revision;
-		} user;
-		struct {
-			uint16_t target_size;
-			void *target;
-		} kernel;
-		uint16_t target_size;
-	} u;
-	unsigned char data[0];
-};
-#endif
-
 struct payload {
 	struct ipt_replace repl;
 	struct ipt_entry ent;
-- 
2.31.1

[LTP] [PATCH v4 3/3] Add setsockopt08, CVE-2021-22555

[Richard Palethorpe]

This is a copy and paste of Nicolai's reproducer. The main difference
is that I moved some code around. Of course I also used LTP library
features, but essentially it works the same.

There are some hard coded values which I do not like. I guess these
could be calculated or varied somehow. However I struggle to understand
what the kernel is doing. This perhaps needs more investigation. We
could try generalising this test and setsockopt03.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Fixes: #850
---
 runtest/cve                                   |   1 +
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/setsockopt/.gitignore     |   1 +
 .../kernel/syscalls/setsockopt/setsockopt08.c | 156 ++++++++++++++++++
 4 files changed, 159 insertions(+)
 create mode 100644 testcases/kernel/syscalls/setsockopt/setsockopt08.c

diff --git a/runtest/cve b/runtest/cve
index 8aa048a40..60bd22f01 100644
--- a/runtest/cve
+++ b/runtest/cve
@@ -65,4 +65,5 @@ cve-2020-14416 pty03
 cve-2020-25705 icmp_rate_limit01
 cve-2020-29373 io_uring02
 cve-2021-3444 bpf_prog05
+cve-2021-22555 setsockopt08 -i 100
 cve-2021-26708 vsock01
diff --git a/runtest/syscalls b/runtest/syscalls
index b379b2d90..2222990fe 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1378,6 +1378,7 @@ setsockopt04 setsockopt04
 setsockopt05 setsockopt05
 setsockopt06 setsockopt06
 setsockopt07 setsockopt07
+setsockopt08 setsockopt08
 
 settimeofday01 settimeofday01
 settimeofday02 settimeofday02
diff --git a/testcases/kernel/syscalls/setsockopt/.gitignore b/testcases/kernel/syscalls/setsockopt/.gitignore
index 1ca5b836b..95a5e43f8 100644
--- a/testcases/kernel/syscalls/setsockopt/.gitignore
+++ b/testcases/kernel/syscalls/setsockopt/.gitignore
@@ -5,3 +5,4 @@
 /setsockopt05
 /setsockopt06
 /setsockopt07
+/setsockopt08
diff --git a/testcases/kernel/syscalls/setsockopt/setsockopt08.c b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
new file mode 100644
index 000000000..f758dcbdc
--- /dev/null
+++ b/testcases/kernel/syscalls/setsockopt/setsockopt08.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 SUSE LLC <rpalethorpe@suse.com>
+ * Based on reproducer by Nicolai Stange based on PoC Andy Nguyen
+ */
+/*\
+ * [Description]
+ *
+ * This will reproduce the bug on x86_64 in 32bit compatibility
+ * mode. It is most reliable with KASAN enabled. Otherwise it relies
+ * on the out-of-bounds write corrupting something which leads to a
+ * crash. It will run in other scenarious, but is not a test for the
+ * CVE.
+ *
+ * See https://google.github.io/security-research/pocs/linux/cve-2021-22555/writeup.html
+ *
+ * Also below is Nicolai's detailed description of the bug itself.
+ *
+ * The problem underlying CVE-2021-22555 fixed by upstream commit
+ * b29c457a6511 ("netfilter: x_tables: fix compat match/target pad
+ * out-of-bound write") is that the (now removed) padding zeroing code
+ * in xt_compat_target_from_user() had been based on the premise that
+ * the user specified ->u.user.target_size, which will be considered
+ * for the target buffer allocation size, is greater or equal than
+ * what's needed to fit the corresponding xt_target instance's
+ * ->targetsize: if OTOH the user specified ->u.user.target_size is
+ * too small, then the memset() destination address calculated by
+ * adding ->targetsize to the payload start will not point at, but
+ * into or even past the padding. For the table's last entry's target
+ * record, this will result in an out-of-bounds write past the
+ * destination buffer allocated for the converted table. The code
+ * below will create a (compat) table such that the converted table's
+ * calculated size will fit exactly into a slab size of 1024 bytes and
+ * that the memset() in xt_compat_target_from_user() will write past
+ * this slab.
+ *
+ * The table will consist of
+ *  - the mandatory struct compat_ipt_replace header,
+ *  - a single entry consisting of
+ *    - the mandatory compat_ipt_entry header
+ *    - a single 'state' match entry of appropriate size for
+ *      controlling the out-of-bounds write when converting
+ *      the target entry following next,
+ *    - a single 'REJECT' target entry.
+ * The kernel will transform this into a buffer containing (in
+ * this order)
+ * - a xt_table_info
+ * - a single entry consisting of
+ *   - its ipt_entry header
+ *   - a single 'state' match entry
+ *   - followed by a single 'REJECT' target entry.
+ *
+ * The expected sizes for the 'state' match entries as well as the
+ * 'REJECT' target are the size of the base header struct (32 bytes)
+ * plus the size of an unsigned int (4 bytes) each. In the course of
+ * the compat => non-compat conversion, the kernel will insert four
+ * bytes of padding after the unsigned int payload (c.f.  'off'
+ * adjustments via xt_compat_match_offset() and
+ * xt_compat_target_offset() in xt_compat_match_from_user() and
+ * xt_compat_target_from_user() resp.). This code is based on the
+ * premise that the user sets the given ->u.user.match_size or
+ * ->u.user.target_size consistent to the COMPAT_XT_ALIGN()ed payload
+ * size as specified by the corresponding xt_match instance's
+ * ->matchsize or xt_target instance's ->targetsize. That is, the
+ * padding gets inserted unconditionally during the transformation,
+ * independent of the actual values of ->u.user.match_size or
+ * ->u.user.target_size and the result ends up getting layed out with
+ * proper alignment only if said values match the expectations. That's
+ * not a problem in itself, but this unconditional insertion of
+ * padding must be taken into account in the match_size calculation
+ * below.
+ *
+ * For the match_size calculation below, note that the chosen
+ * target slab size is 1024 and that
+ *  - sizeof(xt_table_info) = 64
+ *  - sizeof(ipt_entry) = 112
+ *  - the kernel will insert four bytes of padding
+ *    after the match and target entries each.
+ *  - sizeof(struct xt_entry_target) = 32
+ */
+
+#include "tst_test.h"
+#include "tst_safe_net.h"
+#include "lapi/ip_tables.h"
+#include "lapi/namespaces_constants.h"
+
+static void *buffer;
+
+void setup(void)
+{
+	if (tst_kernel_bits() == 32 || sizeof(long) > 4) {
+		tst_res(TCONF,
+			"The vulnerability was only present in 32-bit compat mode");
+	}
+
+	SAFE_UNSHARE(CLONE_NEWUSER);
+	SAFE_UNSHARE(CLONE_NEWNET);
+}
+
+void run(void)
+{
+	struct ipt_replace *ipt_replace = buffer;
+	struct ipt_entry *ipt_entry = &ipt_replace->entries[0];
+	struct xt_entry_match *xt_entry_match =
+		(struct xt_entry_match *)&ipt_entry->elems[0];
+	const size_t tgt_size = 32;
+	const size_t match_size = 1024 - 64 - 112 - 4 - tgt_size - 4;
+	struct xt_entry_target *xt_entry_tgt =
+		((struct xt_entry_target *) (&ipt_entry->elems[0] + match_size));
+	int fd = SAFE_SOCKET(AF_INET, SOCK_DGRAM, 0);
+
+	xt_entry_match->u.user.match_size = (u_int16_t)match_size;
+	strcpy(xt_entry_match->u.user.name, "state");
+
+	xt_entry_tgt->u.user.target_size = (u_int16_t)tgt_size;
+	strcpy(xt_entry_tgt->u.user.name, "REJECT");
+
+	ipt_entry->target_offset =
+		(__builtin_offsetof(struct ipt_entry, elems) + match_size);
+	ipt_entry->next_offset = ipt_entry->target_offset + tgt_size;
+
+	strcpy(ipt_replace->name, "filter");
+	ipt_replace->num_entries = 1;
+	ipt_replace->num_counters = 1;
+	ipt_replace->size = ipt_entry->next_offset;
+
+	TST_EXP_FAIL(setsockopt(fd, IPPROTO_IP, IPT_SO_SET_REPLACE, buffer, 1),
+		     EINVAL,
+		     "setsockopt(%d, IPPROTO_IP, IPT_SO_SET_REPLACE, %p, 1)",
+		     fd, buffer);
+
+	SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.taint_check = TST_TAINT_W | TST_TAINT_D,
+	.forks_child = 1,
+	.bufs = (struct tst_buffers []) {
+		{&buffer, .size = 2048},
+		{},
+	},
+	.needs_kconfigs = (const char *[]) {
+		"CONFIG_NETFILTER_XT_MATCH_STATE",
+		"CONFIG_IP_NF_TARGET_REJECT",
+		"CONFIG_USER_NS=y",
+		"CONFIG_NET_NS=y",
+		NULL
+	},
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "b29c457a6511"},
+		{"CVE", "2021-22555"},
+		{}
+	}
+};
-- 
2.31.1

[LTP] [PATCH v4 3/3] Add setsockopt08, CVE-2021-22555

[Cyril Hrubis]

Hi!
> +#include "tst_test.h"
> +#include "tst_safe_net.h"
> +#include "lapi/ip_tables.h"

I've fixed the failure on Centos with missing IFNAMSIZ but this still
fails to compile on ubuntu xenial because the tst_safe_net.h pull in
netinet/in.h and lapi/ip_tables.h pulls in linux/in.h and on old enough
systems these two headers does not like to be included at the same time.

I'm not sure how to fix this, either we drop the include to
linux/netfilter_ipv4/ip_tables.h completely or we add a configure check
if netinet/in.h and linux/in.h could be included at the same time and
ifdef the linux/netfilter_ipv4/ip_tables.h with that check.

Either way both looks like a hack, if anyone has a better idea please
suggest it.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 3/3] Add setsockopt08, CVE-2021-22555

[Cyril Hrubis]

Hi!
> > +#include "tst_test.h"
> > +#include "tst_safe_net.h"
> > +#include "lapi/ip_tables.h"
> 
> I've fixed the failure on Centos with missing IFNAMSIZ but this still
> fails to compile on ubuntu xenial because the tst_safe_net.h pull in
> netinet/in.h and lapi/ip_tables.h pulls in linux/in.h and on old enough
> systems these two headers does not like to be included at the same time.
> 
> I'm not sure how to fix this, either we drop the include to
> linux/netfilter_ipv4/ip_tables.h completely or we add a configure check
> if netinet/in.h and linux/in.h could be included at the same time and
> ifdef the linux/netfilter_ipv4/ip_tables.h with that check.
> 
> Either way both looks like a hack, if anyone has a better idea please
> suggest it.

Uff and it looks like setsockopt03 does include the same headers so this
could probably be fixed by another shuffle, but I will have to figure
out what has to be moved and where.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 3/3] Add setsockopt08, CVE-2021-22555

[Richard Palethorpe]

Hello Cyril,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > +#include "tst_test.h"
>> > +#include "tst_safe_net.h"
>> > +#include "lapi/ip_tables.h"
>> 
>> I've fixed the failure on Centos with missing IFNAMSIZ but this still
>> fails to compile on ubuntu xenial because the tst_safe_net.h pull in
>> netinet/in.h and lapi/ip_tables.h pulls in linux/in.h and on old enough
>> systems these two headers does not like to be included at the same time.
>> 
>> I'm not sure how to fix this, either we drop the include to
>> linux/netfilter_ipv4/ip_tables.h completely or we add a configure check
>> if netinet/in.h and linux/in.h could be included at the same time and
>> ifdef the linux/netfilter_ipv4/ip_tables.h with that check.
>> 
>> Either way both looks like a hack, if anyone has a better idea please
>> suggest it.
>
> Uff and it looks like setsockopt03 does include the same headers so this
> could probably be fixed by another shuffle, but I will have to figure
> out what has to be moved and where.

Looking in libc-compat.h we have:

/* Coordinate with glibc netinet/in.h header. */
#if defined(_NETINET_IN_H)

/* GLIBC headers included first so don't define anything
 * that would already be defined. */
#define __UAPI_DEF_IN_ADDR		0
...


I suppose we may be able to do something similar. In tst_safe_net.h we
could check if _LINUX_IN_H is defined before including
netinet/in.h. Then include lapi/ip_tables.h first. Maybe even put

#ifdef _NETINET_IN_H
# error "incompatible system header ..."
#endif

in lapi/ip_tables.h

IDK if this may break some existing tests.

-- 
Thank you,
Richard.

[LTP] [PATCH v4 1/3] API: TST_EXP_FAIL: Allow passing when errno is not set

[Cyril Hrubis]

Hi!
Patchset pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 1/3] API: TST_EXP_FAIL: Allow passing when errno is not checked

[Cyril Hrubis]

Hi!
> >> This is also less surprising than giving errno == 0 a dual
> >> meaning.
> >
> > But I do agree that the current if (ERRNO) branch is confusing. I would
> > be for dropping the if (ERRNO) and checking the TST_ERR against ERRNO
> > unconditionally.
> >
> > Also note that the TEST() macro clears errno, so if a syscall fails but
> > does not report any error TST_ERR will end up 0 either way so there is
> > no need for having special handling for 0.
> 
> There is if the errno is set, but is undefined. Like if the resulting
> errno is platform or config dependent.
> 
> In the present case though we can just check for EINVAL. That is what
> the setsockopt man page indicates. Initially I wasn't sure if EFAULT
> were equally valid, but it seems not.

There was also an idea to add another version that would take an array
of possible errnos if that happens, but I fear that the complexity would
be getting out of hand if we keep adding features like that.

> I suppose we can fall back to using TEST() if the other case arises.

Yes.

-- 
Cyril Hrubis
chrubis@suse.cz