lib: Add safe_recvmsg

Signed-off-by Richard Palethorpe <[email protected]>

---

I can not find the original reproducer posted upstream. I assume it was created by syzkaller.

testcases/cve/cve-2017-18075.c new file +21 -0

+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * Test for CVE-2017-5754 - pcrypt mishandles freeing instances
+ *
+ * See commit d76c68109f37 crypto: pcrypt - fix freeing pcrypt instances.
+ *
+ * If the bug is present this will most likely crash your kernel.
+ */
+

Quoted 5 lines · Richard Palethorpe

Signed-off-by Richard Palethorpe <[email protected]>

---

I can not find the original reproducer posted upstream. I assume it was created by syzkaller.

Yes: https://groups.google.com/forum/#!topic/syzkaller-bugs/NKn_ivoPOpk

However, this rewrite to crypto's netlink interface might be different enough such that ...

testcases/cve/cve-2017-18075.c line 7

Quoted 13 lines

testcases/cve/cve-2017-18075.c new file +7 -0

new file mode 100644
index 000000000..3723b0655
@@ -0,0 +1,201 @@
+/*
+ * Copyright (c) 2018 SUSE
+ * Author: Nicolai Stange <[email protected]>
+ * LTP conversion: Richard Palethorpe <[email protected]>
+ *
+ * Based on the reproducer posted upstream so other copyrights may
+ * apply.

... this isn't really needed, but I'm not a lawyer.

Thanks,

Nicolai

Quoted 15 lines

testcases/cve/cve-2017-18075.c new file +13 -0

+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.

testcases/cve/cve-2017-18075.c line 28

Quoted 8 lines

testcases/cve/cve-2017-18075.c new file +8 -0

+ *
+ * Test for CVE-2017-5754 - pcrypt mishandles freeing instances
+ *
+ * See commit d76c68109f37 crypto: pcrypt - fix freeing pcrypt instances.
+ *
+ * If the bug is present this will most likely crash your kernel.
+ */
+

Signed-off-by Richard Palethorpe <[email protected]>

---

I can not find the original reproducer posted upstream. I assume it was created by syzkaller.

This patch will not display error messages correctly without the previous TRERRNO patch I posted.

 runtest/cve                    |   1 +
 testcases/cve/.gitignore       |   1 +
 testcases/cve/cve-2017-18075.c | 201 +++++++++++++++++++++++++++++++++++++++++

testcases/cve/cve-2017-18075.c +0 -0

testcases/cve/cve-2017-18075.c +116 -0

+
+#define ATTEMPTS 10000
+
+enum {
+	CRYPTO_MSG_BASE = 0x10,
+	CRYPTO_MSG_NEWALG = 0x10,
+	CRYPTO_MSG_DELALG,
+	CRYPTO_MSG_UPDATEALG,
+	CRYPTO_MSG_GETALG,
+	CRYPTO_MSG_DELRNG,
+	__CRYPTO_MSG_MAX
+};
+
+#define CRYPTO_MAX_ALG_NAME		64
+#define CRYPTO_MAX_NAME CRYPTO_MAX_ALG_NAME
+
+#define CRYPTO_ALG_TYPE_MASK		0x0000000f
+#define CRYPTO_ALG_TYPE_AEAD		0x00000003
+
+#ifndef NETLINK_CRYPTO
+#define NETLINK_CRYPTO 21
+#endif
+
+struct crypto_user_alg {
+	char cru_name[CRYPTO_MAX_ALG_NAME];
+	char cru_driver_name[CRYPTO_MAX_ALG_NAME];
+	char cru_module_name[CRYPTO_MAX_ALG_NAME];
+	uint32_t cru_type;
+	uint32_t cru_mask;
+	uint32_t cru_refcnt;
+	uint32_t cru_flags;
+};
+
+static void send_nl_msg(int fd, struct nlmsghdr *nh, void *payload)
+{
+	static unsigned int sequence_number;
+	struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
+	struct iovec iov[2] = {
+		{nh, sizeof(*nh)},
+		{payload, nh->nlmsg_len - sizeof(*nh)}
+	};
+	struct msghdr msg = {
+		.msg_name = &sa,
+		.msg_namelen = sizeof(sa),
+		.msg_iov = iov,
+		.msg_iovlen = 2
+	};
+
+	nh->nlmsg_pid = 0;
+	nh->nlmsg_seq = ++sequence_number;
+	/* Request an ack from kernel by setting NLM_F_ACK */
+	nh->nlmsg_flags |= NLM_F_ACK;
+
+	SAFE_SENDMSG(nh->nlmsg_len, fd, &msg, 0);
+}
+
+static int read_reply(int fd)
+{
+	int len;
+	char buf[4096];
+	struct iovec iov = { buf, sizeof(buf) };
+	struct sockaddr_nl sa;
+	struct msghdr msg = {
+		.msg_name = &sa,
+		.msg_namelen = sizeof(sa),
+		.msg_iov = &iov,
+		.msg_iovlen = 1
+	};
+	struct nlmsghdr *nh;
+
+	len = SAFE_RECVMSG(0, fd, &msg, 0);
+
+	for (nh = (struct nlmsghdr *) buf;
+	     NLMSG_OK(nh, len);
+	     nh = NLMSG_NEXT(nh, len)) {
+		/* The end of multipart message. */
+		if (nh->nlmsg_type == NLMSG_DONE)
+			return 0;
+
+		if (nh->nlmsg_type == NLMSG_ERROR)
+			return ((struct nlmsgerr *)NLMSG_DATA(nh))->error;
+	}
+
+	return 0;
+}
+
+static int add_alg(int fd, struct crypto_user_alg *a)
+{
+	int r;
+	struct nlmsghdr nh = {
+		.nlmsg_len = sizeof(struct nlmsghdr) + sizeof(*a),
+		.nlmsg_type = CRYPTO_MSG_NEWALG,
+		.nlmsg_flags = NLM_F_REQUEST,
+	};
+
+	send_nl_msg(fd, &nh, a);
+
+	r = read_reply(fd);
+	if (r == -EEXIST)
+		return 0;
+	return r;
+}
+
+static int del_alg(int fd, struct crypto_user_alg *a)
+{
+	int i;
+	struct timespec delay = { .tv_nsec = 1000000 };
+	struct nlmsghdr nh = {
+		.nlmsg_len = sizeof(struct nlmsghdr) + sizeof(*a),
+		.nlmsg_type = CRYPTO_MSG_DELALG,
+		.nlmsg_flags = NLM_F_REQUEST,
+	};
+
+	for (i = 0; i < 1000; i++) {
+		send_nl_msg(fd, &nh, a);
+

testcases/cve/cve-2017-18075.c +6 -0

+
+void setup(void)
+{
+	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
+}
+

Hi Richard,

Quoted 6 lines · Richard Palethorpe

Signed-off-by Richard Palethorpe <[email protected]>

---

I can not find the original reproducer posted upstream. I assume it was created by syzkaller.

Quoted 6 lines

This patch will not display error messages correctly without the previous TRERRNO patch I posted.

 runtest/cve                    |   1 +
 testcases/cve/.gitignore       |   1 +
 testcases/cve/cve-2017-18075.c | 201 +++++++++++++++++++++++++++++++++++++++++

Thanks for writing an LTP test for this!

Just my 2 cents, but I think it is insane to be naming tests after CVE numbers instead of putting them in an appropriate place, like a crypto/ directory for this one. People aren't going to remember what "CVE-2017-18075" is. I'm even the person who fixed this bug and requested this CVE, and I still didn't recognize the CVE number; this patch only drew my attention because the subject line mentioned pcrypt. (And now I see that I missed the recent test for the modify_ldt() use-after-free bug because the patch subject line and description only mentioned "CVE-2017-17053".)

Note that you can still put the test in the "cve" runtest file even if the test is not in the cve/ directory.

(More comments below)

Quoted 1 line

testcases/cve/cve-2017-18075.c +0 -0

[...]

Quoted 116 lines

testcases/cve/cve-2017-18075.c +116 -0

+
+#define ATTEMPTS 10000
+
+enum {
+	CRYPTO_MSG_BASE = 0x10,
+	CRYPTO_MSG_NEWALG = 0x10,
+	CRYPTO_MSG_DELALG,
+	CRYPTO_MSG_UPDATEALG,
+	CRYPTO_MSG_GETALG,
+	CRYPTO_MSG_DELRNG,
+	__CRYPTO_MSG_MAX
+};
+
+#define CRYPTO_MAX_ALG_NAME		64
+#define CRYPTO_MAX_NAME CRYPTO_MAX_ALG_NAME
+
+#define CRYPTO_ALG_TYPE_MASK		0x0000000f
+#define CRYPTO_ALG_TYPE_AEAD		0x00000003
+
+#ifndef NETLINK_CRYPTO
+#define NETLINK_CRYPTO 21
+#endif
+
+struct crypto_user_alg {
+	char cru_name[CRYPTO_MAX_ALG_NAME];
+	char cru_driver_name[CRYPTO_MAX_ALG_NAME];
+	char cru_module_name[CRYPTO_MAX_ALG_NAME];
+	uint32_t cru_type;
+	uint32_t cru_mask;
+	uint32_t cru_refcnt;
+	uint32_t cru_flags;
+};
+
+static void send_nl_msg(int fd, struct nlmsghdr *nh, void *payload)
+{
+	static unsigned int sequence_number;
+	struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
+	struct iovec iov[2] = {
+		{nh, sizeof(*nh)},
+		{payload, nh->nlmsg_len - sizeof(*nh)}
+	};
+	struct msghdr msg = {
+		.msg_name = &sa,
+		.msg_namelen = sizeof(sa),
+		.msg_iov = iov,
+		.msg_iovlen = 2
+	};
+
+	nh->nlmsg_pid = 0;
+	nh->nlmsg_seq = ++sequence_number;
+	/* Request an ack from kernel by setting NLM_F_ACK */
+	nh->nlmsg_flags |= NLM_F_ACK;
+
+	SAFE_SENDMSG(nh->nlmsg_len, fd, &msg, 0);
+}
+
+static int read_reply(int fd)
+{
+	int len;
+	char buf[4096];
+	struct iovec iov = { buf, sizeof(buf) };
+	struct sockaddr_nl sa;
+	struct msghdr msg = {
+		.msg_name = &sa,
+		.msg_namelen = sizeof(sa),
+		.msg_iov = &iov,
+		.msg_iovlen = 1
+	};
+	struct nlmsghdr *nh;
+
+	len = SAFE_RECVMSG(0, fd, &msg, 0);
+
+	for (nh = (struct nlmsghdr *) buf;
+	     NLMSG_OK(nh, len);
+	     nh = NLMSG_NEXT(nh, len)) {
+		/* The end of multipart message. */
+		if (nh->nlmsg_type == NLMSG_DONE)
+			return 0;
+
+		if (nh->nlmsg_type == NLMSG_ERROR)
+			return ((struct nlmsgerr *)NLMSG_DATA(nh))->error;
+	}
+
+	return 0;
+}
+
+static int add_alg(int fd, struct crypto_user_alg *a)
+{
+	int r;
+	struct nlmsghdr nh = {
+		.nlmsg_len = sizeof(struct nlmsghdr) + sizeof(*a),
+		.nlmsg_type = CRYPTO_MSG_NEWALG,
+		.nlmsg_flags = NLM_F_REQUEST,
+	};
+
+	send_nl_msg(fd, &nh, a);
+
+	r = read_reply(fd);
+	if (r == -EEXIST)
+		return 0;
+	return r;
+}
+
+static int del_alg(int fd, struct crypto_user_alg *a)
+{
+	int i;
+	struct timespec delay = { .tv_nsec = 1000000 };
+	struct nlmsghdr nh = {
+		.nlmsg_len = sizeof(struct nlmsghdr) + sizeof(*a),
+		.nlmsg_type = CRYPTO_MSG_DELALG,
+		.nlmsg_flags = NLM_F_REQUEST,
+	};
+
+	for (i = 0; i < 1000; i++) {
+		send_nl_msg(fd, &nh, a);
+

testcases/cve/cve-2017-18075.c line 162

Quoted 10 lines

testcases/cve/cve-2017-18075.c +10 -0

+		TEST(read_reply(fd));
+		if (TEST_RETURN != -EBUSY)
+			break;
+
+		if (nanosleep(&delay, NULL) && errno != EINTR)
+			tst_brk(TBROK | TERRNO, "nanosleep");
+	}
+
+	return TEST_RETURN;
+}

I suggest putting this NETLINK_CRYPTO stuff in a common location that can be used by other tests too. This will not be the last crypto API bug. The

definitions for AF_ALG probably should be there too; though AF_ALG isn't used by this test, many crypto bugs I've fixed or seen fixed recently are accessible through it. (E.g. see commit ecaaab564978, "crypto: salsa20 - fix blkcipher_walk API usage" or commit e57121d08c38, "crypto: chacha20poly1305 - validate the digest size". Sorry, I was a bit lazy by just putting reproducers in the commit messages and not writing "real" tests.) It would be great to have helper functions in LTP for testing the crypto API, so that they don't have to be repeated in every test.

Quoted 7 lines

testcases/cve/cve-2017-18075.c +6 -0

+
+void setup(void)
+{
+	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
+}
+

testcases/cve/cve-2017-18075.c line 178

Quoted 10 lines

testcases/cve/cve-2017-18075.c +10 -0

+void run(void)
+{
+	int fd, i;
+	struct crypto_user_alg a = {
+		.cru_driver_name = "pcrypt(authenc(hmac(sha256-generic),cbc(aes-generic)))",
+		.cru_type = CRYPTO_ALG_TYPE_AEAD,
+		.cru_mask = CRYPTO_ALG_TYPE_MASK,
+	};
+
+	fd = SAFE_SOCKET(AF_NETLINK, SOCK_DGRAM, NETLINK_CRYPTO);

Does this test get correctly skipped if the needed crypto modules are not enabled? There are several cases:

- NETLINK_CRYPTO unsupported (CONFIG_CRYPTO_USER not set)
- pcrypt unsupported (CONFIG_CRYPTO_PCRYPT not set)
- underlying algorithms unsupported (CONFIG_CRYPTO_AUTHENC, CONFIG_CRYPTO_HMAC, CONFIG_CRYPTO_SHA256, CONFIG_CRYPTO_CBC, or CONFIG_CRYPTO_AES not set)

testcases/cve/cve-2017-18075.c line 201

Quoted 6 lines

testcases/cve/cve-2017-18075.c +6 -0

+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.needs_root = 1,
+	.min_kver = "4.2.0",
+};

Why is a minimum kernel version needed?

- Eric

Quoted 3 lines

 runtest/cve                    |   1 +
 testcases/cve/.gitignore       |   1 +
 testcases/cve/cve-2017-18075.c | 201 +++++++++++++++++++++++++++++++++++++++++

Thanks for writing an LTP test for this!

Just my 2 cents, but I think it is insane to be naming tests after CVE numbers instead of putting them in an appropriate place, like a crypto/ directory for this one. People aren't going to remember what "CVE-2017-18075" is. I'm even the person who fixed this bug and requested this CVE, and I still didn't recognize the CVE number; this patch only drew my attention because the subject line mentioned pcrypt. (And now I see that I missed the recent test for the modify_ldt() use-after-free bug because the patch subject line and description only mentioned "CVE-2017-17053".)

I suggest putting this NETLINK_CRYPTO stuff in a common location that can be used by other tests too. This will not be the last crypto API bug. The definitions for AF_ALG probably should be there too; though AF_ALG isn't used by this test, many crypto bugs I've fixed or seen fixed recently are accessible through it. (E.g. see commit ecaaab564978, "crypto: salsa20 - fix blkcipher_walk API usage" or commit e57121d08c38, "crypto: chacha20poly1305 - validate the digest size". Sorry, I was a bit lazy by just putting reproducers in the commit messages and not writing "real" tests.) It would be great to have helper functions in LTP for testing the crypto API, so that they don't have to be repeated in every test.

Identical to safe_sendmsg except *msg is not const.

Signed-off-by Richard Palethorpe <[email protected]>

---
 include/safe_net_fn.h  |  3 +++
 include/tst_safe_net.h |  3 +++
 lib/safe_net.c         | 23 +++++++++++++++++++++++
 3 files changed, 29 insertions(+)

include/safe_net_fn.h +3 -0

index 2a19be875..89e5bf0cc 100644
@@ -49,6 +49,9 @@ ssize_t safe_sendto(const char *file, const int lineno, char len_strict,
 ssize_t safe_sendmsg(const char *file, const int lineno, size_t msg_len,
 		  int sockfd, const struct msghdr *msg, int flags);
 
+ssize_t safe_recvmsg(const char *file, const int lineno, size_t msg_len,
+		  int sockfd, struct msghdr *msg, int flags);
+
 int safe_bind(const char *file, const int lineno, void (cleanup_fn)(void),
 	      int socket, const struct sockaddr *address,
 	      socklen_t address_len);

include/tst_safe_net.h +3 -0

index 68e9f5cfb..d0c0a1dc5 100644
@@ -54,6 +54,9 @@
 #define SAFE_SENDMSG(msg_len, fd, msg, flags) \
 	safe_sendmsg(__FILE__, __LINE__, msg_len, fd, msg, flags)
 
+#define SAFE_RECVMSG(msg_len, fd, msg, flags)		\
+	safe_recvmsg(__FILE__, __LINE__, msg_len, fd, msg, flags)
+
 #define SAFE_BIND(socket, address, address_len) \
 	safe_bind(__FILE__, __LINE__, NULL, socket, address, \
 		  address_len)

lib/safe_net.c +16 -0

index 9ea9d2b42..5ad478795 100644
@@ -209,6 +209,29 @@ ssize_t safe_sendmsg(const char *file, const int lineno, size_t len,
 	return rval;
 }
 
+ssize_t safe_recvmsg(const char *file, const int lineno, size_t len,
+		     int sockfd, struct msghdr *msg, int flags)
+{
+	ssize_t rval;
+
+	rval = recvmsg(sockfd, msg, flags);
+
+	if (rval == -1) {
+		tst_brkm(TBROK | TERRNO, NULL,
+			 "%s:%d: recvmsg(%d, %p, %d) failed",
+			 file, lineno, sockfd, msg, flags);
+	}
+
+	if (len && (size_t)rval != len) {
+		tst_brkm(TBROK, NULL,
+			 "%s:%d: recvmsg(%d, %p, %d) ret(%zd) != len(%zu)",

Quoted 61 lines

Identical to safe_sendmsg except *msg is not const.

Signed-off-by Richard Palethorpe <[email protected]>

---
 include/safe_net_fn.h  |  3 +++
 include/tst_safe_net.h |  3 +++
 lib/safe_net.c         | 23 +++++++++++++++++++++++
 3 files changed, 29 insertions(+)

include/safe_net_fn.h +3 -0

index 2a19be875..89e5bf0cc 100644
@@ -49,6 +49,9 @@ ssize_t safe_sendto(const char *file, const int lineno, char len_strict,
 ssize_t safe_sendmsg(const char *file, const int lineno, size_t msg_len,
 		  int sockfd, const struct msghdr *msg, int flags);
 
+ssize_t safe_recvmsg(const char *file, const int lineno, size_t msg_len,
+		  int sockfd, struct msghdr *msg, int flags);
+
 int safe_bind(const char *file, const int lineno, void (cleanup_fn)(void),
 	      int socket, const struct sockaddr *address,
 	      socklen_t address_len);

include/tst_safe_net.h +3 -0

index 68e9f5cfb..d0c0a1dc5 100644
@@ -54,6 +54,9 @@
 #define SAFE_SENDMSG(msg_len, fd, msg, flags) \
 	safe_sendmsg(__FILE__, __LINE__, msg_len, fd, msg, flags)
 
+#define SAFE_RECVMSG(msg_len, fd, msg, flags)		\
+	safe_recvmsg(__FILE__, __LINE__, msg_len, fd, msg, flags)
+
 #define SAFE_BIND(socket, address, address_len) \
 	safe_bind(__FILE__, __LINE__, NULL, socket, address, \
 		  address_len)

lib/safe_net.c +16 -0

index 9ea9d2b42..5ad478795 100644
@@ -209,6 +209,29 @@ ssize_t safe_sendmsg(const char *file, const int lineno, size_t len,
 	return rval;
 }
 
+ssize_t safe_recvmsg(const char *file, const int lineno, size_t len,
+		     int sockfd, struct msghdr *msg, int flags)
+{
+	ssize_t rval;
+
+	rval = recvmsg(sockfd, msg, flags);
+
+	if (rval == -1) {
+		tst_brkm(TBROK | TERRNO, NULL,
+			 "%s:%d: recvmsg(%d, %p, %d) failed",
+			 file, lineno, sockfd, msg, flags);
+	}
+
+	if (len && (size_t)rval != len) {
+		tst_brkm(TBROK, NULL,
+			 "%s:%d: recvmsg(%d, %p, %d) ret(%zd) != len(%zu)",

lib/safe_net.c ssize_t safe_sendmsg(const char *file, const int lineno, size_t len, · line 237

Quoted 10 lines

lib/safe_net.c +7 -0

+			 file, lineno, sockfd, msg, flags, rval, len);
+	}
+
+	return rval;
+
+}
+
 int safe_bind(const char *file, const int lineno, void (cleanup_fn)(void),
 	      int socket, const struct sockaddr *address,
 	      socklen_t address_len)

Obviously correct :).

Acked-by Petr Vorel <[email protected]>