[LTP] [PATCH 1/2] lib: Add safe_recvmsg

[LTP] [PATCH 1/2] lib: Add safe_recvmsg

[Richard Palethorpe]

Identical to safe_sendmsg except *msg is not const.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/safe_net_fn.h  |  3 +++
 include/tst_safe_net.h |  3 +++
 lib/safe_net.c         | 23 +++++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/safe_net_fn.h b/include/safe_net_fn.h
index 2a19be875..89e5bf0cc 100644
--- a/include/safe_net_fn.h
+++ b/include/safe_net_fn.h
@@ -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);
diff --git a/include/tst_safe_net.h b/include/tst_safe_net.h
index 68e9f5cfb..d0c0a1dc5 100644
--- a/include/tst_safe_net.h
+++ b/include/tst_safe_net.h
@@ -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)
diff --git a/lib/safe_net.c b/lib/safe_net.c
index 9ea9d2b42..5ad478795 100644
--- a/lib/safe_net.c
+++ b/lib/safe_net.c
@@ -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)",
+			 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)
-- 
2.16.2

[LTP] [PATCH 2/2] Add CVE-2017-18075, pcrypt mishandles freeing instances

[Richard Palethorpe]

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

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 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 testcases/cve/cve-2017-18075.c

diff --git a/runtest/cve b/runtest/cve
index 8b7cbe539..17ef5186d 100644
--- a/runtest/cve
+++ b/runtest/cve
@@ -32,3 +32,4 @@ cve-2017-5754 meltdown
 cve-2017-17052 cve-2017-17052
 cve-2017-16939 cve-2017-16939
 cve-2017-17053 cve-2017-17053
+cve-2017-18075 cve-2017-18075
diff --git a/testcases/cve/.gitignore b/testcases/cve/.gitignore
index b05c0ded6..e4dbb58c1 100644
--- a/testcases/cve/.gitignore
+++ b/testcases/cve/.gitignore
@@ -14,3 +14,4 @@ stack_clash
 cve-2017-17052
 cve-2017-16939
 cve-2017-17053
+cve-2017-18075
diff --git a/testcases/cve/cve-2017-18075.c b/testcases/cve/cve-2017-18075.c
new file mode 100644
index 000000000..3723b0655
--- /dev/null
+++ b/testcases/cve/cve-2017-18075.c
@@ -0,0 +1,201 @@
+/*
+ * Copyright (c) 2018 SUSE
+ * Author: Nicolai Stange <nstange@suse.de>
+ * LTP conversion: Richard Palethorpe <rpalethorpe@suse.com>
+ *
+ * Based on the reproducer posted upstream so other copyrights may
+ * apply.
+ *
+ * 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.
+ */
+
+#include <sys/socket.h>
+#include <linux/netlink.h>
+#include <errno.h>
+#include <time.h>
+
+#include "tst_test.h"
+#include "tst_safe_net.h"
+#include "tst_taint.h"
+
+#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);
+
+		TEST(read_reply(fd));
+		if (TEST_RETURN != -EBUSY)
+			break;
+
+		if (nanosleep(&delay, NULL) && errno != EINTR)
+			tst_brk(TBROK | TERRNO, "nanosleep");
+	}
+
+	return TEST_RETURN;
+}
+
+void setup(void)
+{
+	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
+}
+
+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);
+
+	for (i = 0; i < ATTEMPTS; ++i) {
+		TEST(add_alg(fd, &a));
+		if (TEST_RETURN)
+			tst_brk(TBROK | TRERRNO, "add_alg");
+
+		TEST(del_alg(fd, &a));
+		if (TEST_RETURN)
+			tst_brk(TBROK | TRERRNO, "del_alg");
+	}
+
+	if (tst_taint_check())
+		tst_res(TFAIL, "Kernel has been tainted, check kernel log");
+	else
+		tst_res(TPASS, "You are probably safe");
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.needs_root = 1,
+	.min_kver = "4.2.0",
+};
-- 
2.16.2

[LTP] [PATCH 2/2] Add CVE-2017-18075, pcrypt mishandles freeing instances

[Nicolai Stange]

Richard Palethorpe <rpalethorpe@suse.com> writes:

> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>
> 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 ...


> diff --git a/testcases/cve/cve-2017-18075.c b/testcases/cve/cve-2017-18075.c
> new file mode 100644
> index 000000000..3723b0655
> --- /dev/null
> +++ b/testcases/cve/cve-2017-18075.c
> @@ -0,0 +1,201 @@
> +/*
> + * Copyright (c) 2018 SUSE
> + * Author: Nicolai Stange <nstange@suse.de>
> + * LTP conversion: Richard Palethorpe <rpalethorpe@suse.com>
> + *
> + * Based on the reproducer posted upstream so other copyrights may
> + * apply.

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

Thanks,

Nicolai



> + *
> + * 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.
> + */
> +

-- 
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton,
HRB 21284 (AG Nürnberg)

[LTP] [PATCH 2/2] Add CVE-2017-18075, pcrypt mishandles freeing instances

[Richard Palethorpe]

Hello,

Nicolai Stange writes:

> Richard Palethorpe <rpalethorpe@suse.com> writes:
>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>>
>> 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 ...
>
>
>> diff --git a/testcases/cve/cve-2017-18075.c b/testcases/cve/cve-2017-18075.c
>> new file mode 100644
>> index 000000000..3723b0655
>> --- /dev/null
>> +++ b/testcases/cve/cve-2017-18075.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * Copyright (c) 2018 SUSE
>> + * Author: Nicolai Stange <nstange@suse.de>
>> + * LTP conversion: Richard Palethorpe <rpalethorpe@suse.com>
>> + *
>> + * Based on the reproducer posted upstream so other copyrights may
>> + * apply.
>
> ... this isn't really needed, but I'm not a lawyer.
>
> Thanks,
>
> Nicolai
>

I will just replace it with a link to the Syzkaller reproducer (mainly
just for people's information). Thanks.

>
>
>> + *
>> + * 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.
>> + */
>> +


--
Thank you,
Richard.

[LTP] [PATCH 2/2] Add CVE-2017-18075, pcrypt mishandles freeing instances

[Eric Biggers]

Hi Richard,

On Wed, Mar 14, 2018 at 03:54:27PM +0100, Richard Palethorpe wrote:
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
> 
> 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 +++++++++++++++++++++++++++++++++++++++++

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)

> diff --git a/testcases/cve/cve-2017-18075.c b/testcases/cve/cve-2017-18075.c
[...]
> +
> +#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);
> +
> +		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.

> +
> +void setup(void)
> +{
> +	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
> +}
> +
> +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)

> +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

[LTP] [PATCH 2/2] Add CVE-2017-18075, pcrypt mishandles freeing instances

[Richard Palethorpe]

Hello Eric,

Eric Biggers writes:

> Hi Richard,
>
> On Wed, Mar 14, 2018 at 03:54:27PM +0100, Richard Palethorpe wrote:
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>>
>> 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 +++++++++++++++++++++++++++++++++++++++++
>
> 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.

I'm happy to put it in kernel/crypto. I just wasn't sure where to put it
so submitted it under the CVE directory. I guess I should have CC'ed you.

>
> (More comments below)
>
>> diff --git a/testcases/cve/cve-2017-18075.c b/testcases/cve/cve-2017-18075.c
> [...]
>> +
>> +#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);
>> +
>> +		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.

I suppose we would benefit from some generic netlink helper functions
also. In my netlink.h there are about 21 different protocols defined, so
we could create include/tst_netlink.h and then add
include/tst_netlink_crypto.h and tst_netlink_route.h etc.

Currently we only have a few tests using netlink and they are mostly
doing their own thing.

>
>> +
>> +void setup(void)
>> +{
>> +	tst_taint_init(TST_TAINT_W | TST_TAINT_D);
>> +}
>> +
>> +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)
>
>> +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

So this is wrong then, I need to check the errno of socket() when it
fails and can remove the min_kver which I think was the kernel version
where the feature was introduced.

--
Thank you,
Richard.

[LTP] [PATCH 2/2] Add CVE-2017-18075, pcrypt mishandles freeing instances

[Cyril Hrubis]

Hi!
> >  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".)

Agreed, the numbers suck, I have to read everything twice to avoid
typos.

Maybe we should name the test files after the kernel subsystem with an
increasing counter as a last resort.

> 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.

We do have include/lapi/ headers for that purpose, we may as well put it
there.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] lib: Add safe_recvmsg

[Petr Vorel]

Hi,

> Identical to safe_sendmsg except *msg is not const.

> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  include/safe_net_fn.h  |  3 +++
>  include/tst_safe_net.h |  3 +++
>  lib/safe_net.c         | 23 +++++++++++++++++++++++
>  3 files changed, 29 insertions(+)

> diff --git a/include/safe_net_fn.h b/include/safe_net_fn.h
> index 2a19be875..89e5bf0cc 100644
> --- a/include/safe_net_fn.h
> +++ b/include/safe_net_fn.h
> @@ -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);
> diff --git a/include/tst_safe_net.h b/include/tst_safe_net.h
> index 68e9f5cfb..d0c0a1dc5 100644
> --- a/include/tst_safe_net.h
> +++ b/include/tst_safe_net.h
> @@ -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)
> diff --git a/lib/safe_net.c b/lib/safe_net.c
> index 9ea9d2b42..5ad478795 100644
> --- a/lib/safe_net.c
> +++ b/lib/safe_net.c
> @@ -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)",
> +			 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 <pvorel@suse.cz>


Kind regards,
Petr

[LTP] [PATCH 1/2] lib: Add safe_recvmsg

[Petr Vorel]

Hi,

> > Identical to safe_sendmsg except *msg is not const.

> > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> > ---
> >  include/safe_net_fn.h  |  3 +++
> >  include/tst_safe_net.h |  3 +++
> >  lib/safe_net.c         | 23 +++++++++++++++++++++++
> >  3 files changed, 29 insertions(+)

> > diff --git a/include/safe_net_fn.h b/include/safe_net_fn.h
> > index 2a19be875..89e5bf0cc 100644
> > --- a/include/safe_net_fn.h
> > +++ b/include/safe_net_fn.h
> > @@ -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);
> > diff --git a/include/tst_safe_net.h b/include/tst_safe_net.h
> > index 68e9f5cfb..d0c0a1dc5 100644
> > --- a/include/tst_safe_net.h
> > +++ b/include/tst_safe_net.h
> > @@ -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)
> > diff --git a/lib/safe_net.c b/lib/safe_net.c
> > index 9ea9d2b42..5ad478795 100644
> > --- a/lib/safe_net.c
> > +++ b/lib/safe_net.c
> > @@ -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)",
> > +			 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 <pvorel@suse.cz>

Pushed, thanks!

Kind regards,
Petr