ltp: new test for crypto_user information leak bug

PATCH Superseded Eric Biggers 4 participants 13 messages Dec 6, 2018 – Dec 11, 2018

lore

Share & tools

lore

Eric Biggers

author

2018-12-06 18:18 UTC #0 lore raw

cover ltp: new test for crypto_user information leak bug

This series adds a test for an information leak bug in the crypto user configuration API which was recently fixed.

Eric Biggers (3):
lapi/cryptouser.h: add more declarations
tst_netlink: inline functions in header crypto/crypto_user01.c: new test for information leak bug

 include/lapi/cryptouser.h               |  73 +++++++++
 include/tst_netlink.h                   |  11 +-
 runtest/crypto                          |   1 +
 runtest/cve                             |   2 +
 testcases/kernel/crypto/.gitignore      |   1 +
 testcases/kernel/crypto/crypto_user01.c | 208 ++++++++++++++++++++++++
 6 files changed, 291 insertions(+), 5 deletions(-)
 create mode 100644 testcases/kernel/crypto/crypto_user01.c

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Eric Biggers

author

2018-12-06 18:18 UTC #1 lore raw

PATCH 1/3 lapi/cryptouser.h: add more declarations

From: Eric Biggers <[email protected]>

Add some missing declarations for the crypto user configuration API, a.k.a. NETLINK_CRYPTO.

Signed-off-by Eric Biggers <[email protected]>

---
 include/lapi/cryptouser.h | 73 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

include/lapi/cryptouser.h +73 -0

index 61ff21ad3..94d641446 100644
@@ -34,6 +34,24 @@ enum {
 	__CRYPTO_MSG_MAX
 };
 
+enum crypto_attr_type_t {
+	CRYPTOCFGA_UNSPEC,
+	CRYPTOCFGA_PRIORITY_VAL,	/* uint32_t */
+	CRYPTOCFGA_REPORT_LARVAL,	/* struct crypto_report_larval */
+	CRYPTOCFGA_REPORT_HASH,		/* struct crypto_report_hash */
+	CRYPTOCFGA_REPORT_BLKCIPHER,	/* struct crypto_report_blkcipher */
+	CRYPTOCFGA_REPORT_AEAD,		/* struct crypto_report_aead */
+	CRYPTOCFGA_REPORT_COMPRESS,	/* struct crypto_report_comp */
+	CRYPTOCFGA_REPORT_RNG,		/* struct crypto_report_rng */
+	CRYPTOCFGA_REPORT_CIPHER,	/* struct crypto_report_cipher */
+	CRYPTOCFGA_REPORT_AKCIPHER,	/* struct crypto_report_akcipher */
+	CRYPTOCFGA_REPORT_KPP,		/* struct crypto_report_kpp */
+	CRYPTOCFGA_REPORT_ACOMP,	/* struct crypto_report_acomp */
+	__CRYPTOCFGA_MAX
+
+#define CRYPTOCFGA_MAX (__CRYPTOCFGA_MAX - 1)
+};
+
 struct crypto_user_alg {
 	char cru_name[CRYPTO_MAX_NAME];
 	char cru_driver_name[CRYPTO_MAX_NAME];

@@ -44,6 +62,61 @@ struct crypto_user_alg {
 	uint32_t cru_flags;
 };
 
+struct crypto_report_larval {
+	char type[CRYPTO_MAX_NAME];
+};
+
+struct crypto_report_hash {
+	char type[CRYPTO_MAX_NAME];
+	unsigned int blocksize;
+	unsigned int digestsize;
+};
+
+struct crypto_report_cipher {
+	char type[CRYPTO_MAX_NAME];
+	unsigned int blocksize;
+	unsigned int min_keysize;
+	unsigned int max_keysize;
+};
+
+struct crypto_report_blkcipher {
+	char type[CRYPTO_MAX_NAME];
+	char geniv[CRYPTO_MAX_NAME];
+	unsigned int blocksize;
+	unsigned int min_keysize;
+	unsigned int max_keysize;
+	unsigned int ivsize;
+};
+
+struct crypto_report_aead {
+	char type[CRYPTO_MAX_NAME];
+	char geniv[CRYPTO_MAX_NAME];
+	unsigned int blocksize;
+	unsigned int maxauthsize;
+	unsigned int ivsize;
+};
+
+struct crypto_report_comp {
+	char type[CRYPTO_MAX_NAME];
+};
+
+struct crypto_report_rng {
+	char type[CRYPTO_MAX_NAME];
+	unsigned int seedsize;
+};
+
+struct crypto_report_akcipher {
+	char type[CRYPTO_MAX_NAME];
+};
+
+struct crypto_report_kpp {
+	char type[CRYPTO_MAX_NAME];
+};
+
+struct crypto_report_acomp {
+	char type[CRYPTO_MAX_NAME];
+};
+
 #endif	/* HAVE_LINUX_CRYPTOUSER_H */
 
 /* These are taken from include/crypto.h in the kernel tree. They are not

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Eric Biggers

author

2018-12-06 18:18 UTC #2 lore raw

PATCH 2/3 tst_netlink: inline functions in header

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

safe_netlink_send() and safe_netlink_recv() need to be static inline, since they're defined in a .h file.

Signed-off-by Eric Biggers <[email protected]>

---
 include/tst_netlink.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

include/tst_netlink.h +6 -5

index 42232a169..892ef8f78 100644
@@ -33,9 +33,9 @@
 #endif
 
 /** @private */
-ssize_t safe_netlink_send(const char *file, const int lineno,
-			  int fd, const struct nlmsghdr *nh,
-			  const void *payload)
+static inline ssize_t safe_netlink_send(const char *file, const int lineno,
+					int fd, const struct nlmsghdr *nh,
+					const void *payload)
 {
 	struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
 	struct iovec iov[2] = {

@@ -68,8 +68,9 @@ ssize_t safe_netlink_send(const char *file, const int lineno,
 	safe_netlink_send(__FILE__, __LINE__, fd, nl_header, payload)
 
 /** @private */
-ssize_t safe_netlink_recv(const char *file, const int lineno,
-			  int fd, char *nl_headers_buf, size_t buf_len)
+static inline ssize_t safe_netlink_recv(const char *file, const int lineno,
+					int fd, char *nl_headers_buf,
+					size_t buf_len)
 {
 	struct iovec iov = { nl_headers_buf, buf_len };
 	struct sockaddr_nl sa;

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Eric Biggers

author

2018-12-06 18:18 UTC #3 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Test for a bug in the crypto user configuration API (NETLINK_CRYPTO) that leaked uninitialized memory to userspace. This bug was assigned CVE-2018-19854, and it was also a re-introduction of CVE-2013-2547.

Signed-off-by Eric Biggers <[email protected]>

---
 runtest/crypto                          |   1 +
 runtest/cve                             |   2 +
 testcases/kernel/crypto/.gitignore      |   1 +
 testcases/kernel/crypto/crypto_user01.c | 208 ++++++++++++++++++++++++
 4 files changed, 212 insertions(+)
 create mode 100644 testcases/kernel/crypto/crypto_user01.c

runtest/crypto +1 -0

index e5ba61e5e..cdbc44cc8 100644
@@ -1 +1,2 @@
 pcrypt_aead01 pcrypt_aead01
+crypto_user01 crypto_user01

runtest/cve +2 -0

index c4ba74186..78a5d8db2 100644
@@ -3,6 +3,7 @@ cve-2011-0999 thp01 -I 120
 cve-2011-2183 ksm05 -I 10
 cve-2011-2496 vma03
 cve-2012-0957 uname04
+cve-2013-2547 crypto_user01
 cve-2014-0196 cve-2014-0196
 cve-2015-0235 gethostbyname_r01
 cve-2015-7550 keyctl02

@@ -36,3 +37,4 @@ cve-2017-17053 cve-2017-17053
 cve-2017-18075 pcrypt_aead01
 cve-2018-5803 sctp_big_chunk
 cve-2018-1000001 realpath01
+cve-2018-19854 crypto_user01

testcases/kernel/crypto/.gitignore +1 -0

index fafe5c972..759592fbd 100644
@@ -1 +1,2 @@
 pcrypt_aead01
+crypto_user01

testcases/kernel/crypto/crypto_user01.c new file +208 -0

208 lines not on this page · raw message

Signature · 1 line

2.20.0.rc1.387.gf8505762e3-goog

Richard Palethorpe

2018-12-07 14:11 UTC #4 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

Hello,

Quoted 94 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 runtest/crypto                          |   1 +
 runtest/cve                             |   2 +
 testcases/kernel/crypto/.gitignore      |   1 +
 testcases/kernel/crypto/crypto_user01.c | 208 ++++++++++++++++++++++++
 4 files changed, 212 insertions(+)
 create mode 100644 testcases/kernel/crypto/crypto_user01.c

runtest/crypto +1 -0

index e5ba61e5e..cdbc44cc8 100644
@@ -1 +1,2 @@
 pcrypt_aead01 pcrypt_aead01
+crypto_user01 crypto_user01

runtest/cve +2 -0

index c4ba74186..78a5d8db2 100644
@@ -3,6 +3,7 @@ cve-2011-0999 thp01 -I 120
 cve-2011-2183 ksm05 -I 10
 cve-2011-2496 vma03
 cve-2012-0957 uname04
+cve-2013-2547 crypto_user01
 cve-2014-0196 cve-2014-0196
 cve-2015-0235 gethostbyname_r01
 cve-2015-7550 keyctl02

@@ -36,3 +37,4 @@ cve-2017-17053 cve-2017-17053
 cve-2017-18075 pcrypt_aead01
 cve-2018-5803 sctp_big_chunk
 cve-2018-1000001 realpath01
+cve-2018-19854 crypto_user01

testcases/kernel/crypto/.gitignore +1 -0

index fafe5c972..759592fbd 100644
@@ -1 +1,2 @@
 pcrypt_aead01
+crypto_user01

testcases/kernel/crypto/crypto_user01.c new file +42 -0

new file mode 100644
index 000000000..b648fcbdc
@@ -0,0 +1,208 @@
+/*
+ * Copyright 2018 Google LLC
+ *
+ * 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/>.
+ */
+
+/*
+ * Regression test for commit f43f39958beb ("crypto: user - fix leaking
+ * uninitialized memory to userspace"), or CVE-2018-19854; it was also a
+ * re-introduction of CVE-2013-2547.  This bug caused uninitialized kernel stack
+ * memory to be leaked in some string fields in the replies to CRYPTO_MSG_GETALG
+ * messages over NETLINK_CRYPTO.  To try to detect the bug, this test dumps all
+ * algorithms using NLM_F_DUMP mode and checks all string fields for unexpected
+ * nonzero bytes.
+ */
+
+#include <stdlib.h>
+#include <linux/rtnetlink.h>
+
+#include "tst_test.h"
+#include "tst_crypto.h"
+#include "tst_netlink.h"
+
+static struct tst_crypto_session ses = TST_CRYPTO_SESSION_INIT;
+
+static void setup(void)
+{
+	tst_crypto_open(&ses);
+}
+
+static void __check_for_leaks(const char *name, const char *value,

Quoted 1 line

size_t vlen)

IIRC we don't allow functions beginning with underscores because they are reserved by the compiler or C library.

Quoted 27 lines

testcases/kernel/crypto/crypto_user01.c new file +27 -0

+{
+	size_t i;
+
+	for (i = strnlen(value, vlen); i < vlen; i++) {
+		if (value[i] != '\0')
+			tst_brk(TFAIL, "information leak in field '%s'", name);
+	}
+}
+
+#define check_for_leaks(name, field)  \
+	__check_for_leaks(name, field, sizeof(field))
+
+static void validate_one_alg(const struct nlmsghdr *nh)
+{
+	const struct crypto_user_alg *alg = NLMSG_DATA(nh);
+	const struct rtattr *rta = (void *)alg + NLMSG_ALIGN(sizeof(*alg));
+	const struct rtattr *tb[CRYPTOCFGA_MAX + 1] = { 0 };
+	size_t remaining = NLMSG_PAYLOAD(nh, sizeof(*alg));
+
+	check_for_leaks("crypto_user_alg::cru_name", alg->cru_name);
+	check_for_leaks("crypto_user_alg::cru_driver_name",
+			alg->cru_driver_name);
+	check_for_leaks("crypto_user_alg::cru_module_name",
+			alg->cru_module_name);
+
+	while (RTA_OK(rta, remaining)) {
+		if (rta->rta_type < CRYPTOCFGA_MAX &&

Quoted 1 line

!tb[rta->rta_type])

So does this mean we get multiple instances of the same RTA type and we just check the first? If so I wonder if there is any advantage to testing all of them?

Signature · 2 lines

Thank you,
Richard.

Cyril Hrubis

2018-12-10 12:24 UTC #5 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

Hi!

Quoted 5 lines

Quoted 2 lines
+static void __check_for_leaks(const char *name, const char *value, size_t vlen)
IIRC we don't allow functions beginning with underscores because they are reserved by the compiler or C library.

We do append underscores at the end of the identifiers in order not to redefine symbols from compiler/libc.

Signature · 2 lines

Cyril Hrubis
[email protected]

Eric Biggers

author

2018-12-11 05:40 UTC #6 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

In reply to Richard Palethorpe

Hi Richard,

Quoted 127 lines · Richard Palethorpe

Hello,

Quoted 94 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 runtest/crypto                          |   1 +
 runtest/cve                             |   2 +
 testcases/kernel/crypto/.gitignore      |   1 +
 testcases/kernel/crypto/crypto_user01.c | 208 ++++++++++++++++++++++++
 4 files changed, 212 insertions(+)
 create mode 100644 testcases/kernel/crypto/crypto_user01.c

runtest/crypto +1 -0

index e5ba61e5e..cdbc44cc8 100644
@@ -1 +1,2 @@
 pcrypt_aead01 pcrypt_aead01
+crypto_user01 crypto_user01

runtest/cve +2 -0

index c4ba74186..78a5d8db2 100644
@@ -3,6 +3,7 @@ cve-2011-0999 thp01 -I 120
 cve-2011-2183 ksm05 -I 10
 cve-2011-2496 vma03
 cve-2012-0957 uname04
+cve-2013-2547 crypto_user01
 cve-2014-0196 cve-2014-0196
 cve-2015-0235 gethostbyname_r01
 cve-2015-7550 keyctl02

@@ -36,3 +37,4 @@ cve-2017-17053 cve-2017-17053
 cve-2017-18075 pcrypt_aead01
 cve-2018-5803 sctp_big_chunk
 cve-2018-1000001 realpath01
+cve-2018-19854 crypto_user01

testcases/kernel/crypto/.gitignore +1 -0

index fafe5c972..759592fbd 100644
@@ -1 +1,2 @@
 pcrypt_aead01
+crypto_user01

testcases/kernel/crypto/crypto_user01.c new file +42 -0

new file mode 100644
index 000000000..b648fcbdc
@@ -0,0 +1,208 @@
+/*
+ * Copyright 2018 Google LLC
+ *
+ * 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/>.
+ */
+
+/*
+ * Regression test for commit f43f39958beb ("crypto: user - fix leaking
+ * uninitialized memory to userspace"), or CVE-2018-19854; it was also a
+ * re-introduction of CVE-2013-2547.  This bug caused uninitialized kernel stack
+ * memory to be leaked in some string fields in the replies to CRYPTO_MSG_GETALG
+ * messages over NETLINK_CRYPTO.  To try to detect the bug, this test dumps all
+ * algorithms using NLM_F_DUMP mode and checks all string fields for unexpected
+ * nonzero bytes.
+ */
+
+#include <stdlib.h>
+#include <linux/rtnetlink.h>
+
+#include "tst_test.h"
+#include "tst_crypto.h"
+#include "tst_netlink.h"
+
+static struct tst_crypto_session ses = TST_CRYPTO_SESSION_INIT;
+
+static void setup(void)
+{
+	tst_crypto_open(&ses);
+}
+
+static void __check_for_leaks(const char *name, const char *value,

Quoted 1 line

size_t vlen)

IIRC we don't allow functions beginning with underscores because they are reserved by the compiler or C library.

Quoted 24 lines

testcases/kernel/crypto/crypto_user01.c new file +24 -0

+{
+	size_t i;
+
+	for (i = strnlen(value, vlen); i < vlen; i++) {
+		if (value[i] != '\0')
+			tst_brk(TFAIL, "information leak in field '%s'", name);
+	}
+}
+
+#define check_for_leaks(name, field)  \
+	__check_for_leaks(name, field, sizeof(field))
+
+static void validate_one_alg(const struct nlmsghdr *nh)
+{
+	const struct crypto_user_alg *alg = NLMSG_DATA(nh);
+	const struct rtattr *rta = (void *)alg + NLMSG_ALIGN(sizeof(*alg));
+	const struct rtattr *tb[CRYPTOCFGA_MAX + 1] = { 0 };
+	size_t remaining = NLMSG_PAYLOAD(nh, sizeof(*alg));
+
+	check_for_leaks("crypto_user_alg::cru_name", alg->cru_name);
+	check_for_leaks("crypto_user_alg::cru_driver_name",
+			alg->cru_driver_name);
+	check_for_leaks("crypto_user_alg::cru_module_name",
+			alg->cru_module_name);

Quoted 9 lines

Quoted 4 lines
testcases/kernel/crypto/crypto_user01.c new file +3 -0
+
+	while (RTA_OK(rta, remaining)) {
+		if (rta->rta_type < CRYPTOCFGA_MAX &&
!tb[rta->rta_type])
So does this mean we get multiple instances of the same RTA type and we just check the first? If so I wonder if there is any advantage to testing all of them?

I don't believe the API returns multiple of any type currently, but we can check all just as easily, so I'll do it that way instead. Thanks!

- Eric

Richard Palethorpe

2018-12-07 14:30 UTC #7 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

In reply to Eric Biggers

Hello again,

Quoted 76 lines · Eric Biggers

From: Eric Biggers <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 runtest/crypto                          |   1 +
 runtest/cve                             |   2 +
 testcases/kernel/crypto/.gitignore      |   1 +
 testcases/kernel/crypto/crypto_user01.c | 208 ++++++++++++++++++++++++
 4 files changed, 212 insertions(+)
 create mode 100644 testcases/kernel/crypto/crypto_user01.c

runtest/crypto +1 -0

index e5ba61e5e..cdbc44cc8 100644
@@ -1 +1,2 @@
 pcrypt_aead01 pcrypt_aead01
+crypto_user01 crypto_user01

runtest/cve +2 -0

index c4ba74186..78a5d8db2 100644
@@ -3,6 +3,7 @@ cve-2011-0999 thp01 -I 120
 cve-2011-2183 ksm05 -I 10
 cve-2011-2496 vma03
 cve-2012-0957 uname04
+cve-2013-2547 crypto_user01
 cve-2014-0196 cve-2014-0196
 cve-2015-0235 gethostbyname_r01
 cve-2015-7550 keyctl02

@@ -36,3 +37,4 @@ cve-2017-17053 cve-2017-17053
 cve-2017-18075 pcrypt_aead01
 cve-2018-5803 sctp_big_chunk
 cve-2018-1000001 realpath01
+cve-2018-19854 crypto_user01

testcases/kernel/crypto/.gitignore +1 -0

index fafe5c972..759592fbd 100644
@@ -1 +1,2 @@
 pcrypt_aead01
+crypto_user01

testcases/kernel/crypto/crypto_user01.c new file +24 -0

new file mode 100644
index 000000000..b648fcbdc
@@ -0,0 +1,208 @@
+/*
+ * Copyright 2018 Google LLC
+ *
+ * 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/>.
+ */
+
+/*
+ * Regression test for commit f43f39958beb ("crypto: user - fix leaking
+ * uninitialized memory to userspace"), or CVE-2018-19854; it was also a
+ * re-introduction of CVE-2013-2547.  This bug caused uninitialized kernel stack
+ * memory to be leaked in some string fields in the replies to CRYPTO_MSG_GETALG
+ * messages over NETLINK_CRYPTO.  To try to detect the bug, this test dumps all
+ * algorithms using NLM_F_DUMP mode and checks all string fields for unexpected

testcases/kernel/crypto/crypto_user01.c line 34

Quoted 10 lines

testcases/kernel/crypto/crypto_user01.c new file +10 -0

+ * nonzero bytes.
+ */
+
+#include <stdlib.h>
+#include <linux/rtnetlink.h>
+
+#include "tst_test.h"
+#include "tst_crypto.h"
+#include "tst_netlink.h"
+

It seems that on SLE11 there is a bug in the kernel headers which means compilation fails if you include linux/rtnetlink.h before linux/netlink.h. If you switch the order then it compiles OK.

Signature · 2 lines

Thank you,
Richard.

Petr Vorel

2018-12-07 16:34 UTC #8 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

Hi Eric, Richard,

Quoted 9 lines

testcases/kernel/crypto/crypto_user01.c line 33
Quoted 5 lines
testcases/kernel/crypto/crypto_user01.c new file +5 -0
+#include <linux/rtnetlink.h>
+
+#include "tst_test.h"
+#include "tst_crypto.h"
+#include "tst_netlink.h"
It seems that on SLE11 there is a bug in the kernel headers which means compilation fails if you include linux/rtnetlink.h before linux/netlink.h. If you switch the order then it compiles OK.

Correct, it suffers from bug:
https://www.spinics.net/lists/netdev/msg171764.html
How about adding include <linux/rtnetlink.h> into include/tst_netlink.h?

Quoted 5 lines

Quoted 2 lines
+static void __check_for_leaks(const char *name, const char *value, size_t vlen)
IIRC we don't allow functions beginning with underscores because they are reserved by the compiler or C library.

Agree.

Kind regards,
Petr

Richard Palethorpe

2018-12-10 08:59 UTC #9 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

Hello,

Quoted 12 lines · Petr Vorel

Hi Eric, Richard,
Quoted 9 lines
testcases/kernel/crypto/crypto_user01.c line 33
Quoted 5 lines
testcases/kernel/crypto/crypto_user01.c new file +5 -0
+#include <linux/rtnetlink.h>
+
+#include "tst_test.h"
+#include "tst_crypto.h"
+#include "tst_netlink.h"
It seems that on SLE11 there is a bug in the kernel headers which means compilation fails if you include linux/rtnetlink.h before linux/netlink.h. If you switch the order then it compiles OK.

Quoted 4 lines

Correct, it suffers from bug:
https://www.spinics.net/lists/netdev/msg171764.html
How about adding include <linux/rtnetlink.h> into
include/tst_netlink.h?

I think we would also have to include sys/socket.h in tst_netlink.h to really solve the problem.

I am not sure if that is a good idea.

Quoted 11 lines

Quoted 6 lines
Quoted 3 lines
+static void __check_for_leaks(const char *name, const char *value, size_t vlen)
IIRC we don't allow functions beginning with underscores because they are reserved by the compiler or C library.
Agree.
Kind regards,
Petr

--
Thank you,
Richard.

Eric Biggers

author

2018-12-11 05:37 UTC #10 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

Quoted 21 lines · Richard Palethorpe

Hello,
Quoted 12 lines · Petr Vorel
Hi Eric, Richard,
Quoted 9 lines
testcases/kernel/crypto/crypto_user01.c line 33
Quoted 5 lines
testcases/kernel/crypto/crypto_user01.c new file +5 -0
+#include <linux/rtnetlink.h>
+
+#include "tst_test.h"
+#include "tst_crypto.h"
+#include "tst_netlink.h"
It seems that on SLE11 there is a bug in the kernel headers which means compilation fails if you include linux/rtnetlink.h before linux/netlink.h. If you switch the order then it compiles OK.
Quoted 4 lines
Correct, it suffers from bug:
https://www.spinics.net/lists/netdev/msg171764.html
How about adding include <linux/rtnetlink.h> into
include/tst_netlink.h?

Quoted 4 lines

I think we would also have to include sys/socket.h in tst_netlink.h to really solve the problem.
I am not sure if that is a good idea.

tst_netlink.h already has an implicit dependency on safe_net_fn.h which already includes <sys/socket.h>. So it appears the real issue is including <linux/rtnetlink.h> (which includes <linux/netlink.h>) before tst_test.h.

But various other <linux/*> headers include <linux/netlink.h> too, and I don't think all should be included in tst_netlink.h, so I guess I'll just move the include within the test .c file itself...

BTW, I have no system to reproduce this problem on, so you'll just have to tell me whether it works. I tried CentOS 6, but this is already fixed there.

- Eric

Petr Vorel

2018-12-11 10:55 UTC #11 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

Hi Eric, Richard,

...

Quoted 10 lines

Quoted 9 lines
Quoted 4 lines
Correct, it suffers from bug:
https://www.spinics.net/lists/netdev/msg171764.html
How about adding include <linux/rtnetlink.h> into
include/tst_netlink.h?
I think we would also have to include sys/socket.h in tst_netlink.h to really solve the problem.
I am not sure if that is a good idea.

Quoted 7 lines

tst_netlink.h already has an implicit dependency on safe_net_fn.h which already includes <sys/socket.h>. So it appears the real issue is including <linux/rtnetlink.h> (which includes <linux/netlink.h>) before tst_test.h.
But various other <linux/*> headers include <linux/netlink.h> too, and I don't think all should be included in tst_netlink.h, so I guess I'll just move the include within the test .c file itself...

If it's the only change, we can do it before merge (no need to repost whole patchset).

Quoted 2 lines

BTW, I have no system to reproduce this problem on, so you'll just have to tell me whether it works. I tried CentOS 6, but this is already fixed there.

Sure, we'll test it (I can reproduce it as well).

Quoted 1 line

- Eric

Petr

Cyril Hrubis

2018-12-10 12:38 UTC #12 lore raw

PATCH 3/3 crypto/crypto_user01.c: new test for information leak bug

In reply to Eric Biggers

Hi!

Quoted 8 lines

+/*
+ * Copyright 2018 Google LLC
+ *
+ * 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.
+ *

Quoted 8 lines

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

We started to adopt the SPDX licencse identifier instead of this license text because it's shorter and machine parseable.

In this case it would shorten the license to:

Other than the minor issues pointed out by ritchie and peter the code looks good to me.

Signature · 2 lines

Cyril Hrubis
[email protected]