public inbox for ltp@lists.linux.it
 help / color / mirror / Atom feed
* [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs
@ 2017-10-12 20:42 Eric Biggers
  2017-10-12 20:42 ` [LTP] [PATCH v2 1/4] lapi/keyctl.h: add a few missing definitions Eric Biggers
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Eric Biggers @ 2017-10-12 20:42 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

Add tests to LTP for a few more longstanding bugs which were recently
fixed in the kernel keyrings API.

Changed since v1:

    - Updated add_key03 to find an unused user by searching the passwd
      database, which allowed simplifying the test.

    - Added CVE number for keyctl07.

Eric Biggers (4):
  lapi/keyctl.h: add a few missing definitions
  syscalls/keyctl06: new test for keyring_read() buffer overrun
  syscalls/keyctl07: new test for oops when reading negative key
  syscalls/add_key03: new test for forging user keyrings

 include/lapi/keyctl.h                         | 20 ++++++
 runtest/cve                                   |  1 +
 runtest/syscalls                              |  3 +
 testcases/kernel/syscalls/.gitignore          |  3 +
 testcases/kernel/syscalls/add_key/add_key03.c | 98 +++++++++++++++++++++++++++
 testcases/kernel/syscalls/keyctl/keyctl06.c   | 68 +++++++++++++++++++
 testcases/kernel/syscalls/keyctl/keyctl07.c   | 89 ++++++++++++++++++++++++
 7 files changed, 282 insertions(+)
 create mode 100644 testcases/kernel/syscalls/add_key/add_key03.c
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl06.c
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl07.c

-- 
2.15.0.rc0.271.g36b669edcc-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [LTP] [PATCH v2 1/4] lapi/keyctl.h: add a few missing definitions
  2017-10-12 20:42 [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs Eric Biggers
@ 2017-10-12 20:42 ` Eric Biggers
  2017-10-12 20:42 ` [LTP] [PATCH v2 2/4] syscalls/keyctl06: new test for keyring_read() buffer overrun Eric Biggers
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Eric Biggers @ 2017-10-12 20:42 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

KEYCTL_REVOKE, KEYCTL_READ, KEY_SPEC_PROCESS_KEYRING, and
KEY_SPEC_USER_SESSION_KEYRING are used by existing tests.
KEY_SPEC_USER_KEYRING will be used by an upcoming test.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/lapi/keyctl.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/lapi/keyctl.h b/include/lapi/keyctl.h
index 3e7ce4708..73f4fdc90 100644
--- a/include/lapi/keyctl.h
+++ b/include/lapi/keyctl.h
@@ -81,6 +81,10 @@ static inline long keyctl(int cmd, ...)
 # define KEYCTL_UPDATE 2
 #endif
 
+#ifndef KEYCTL_REVOKE
+# define KEYCTL_REVOKE 3
+#endif
+
 #ifndef KEYCTL_SETPERM
 # define KEYCTL_SETPERM 5
 #endif
@@ -89,14 +93,30 @@ static inline long keyctl(int cmd, ...)
 # define KEYCTL_UNLINK 9
 #endif
 
+#ifndef KEYCTL_READ
+# define KEYCTL_READ 11
+#endif
+
 #ifndef KEY_SPEC_THREAD_KEYRING
 # define KEY_SPEC_THREAD_KEYRING -1
 #endif
 
+#ifndef KEY_SPEC_PROCESS_KEYRING
+# define KEY_SPEC_PROCESS_KEYRING -2
+#endif
+
 #ifndef KEY_SPEC_SESSION_KEYRING
 # define KEY_SPEC_SESSION_KEYRING -3
 #endif
 
+#ifndef KEY_SPEC_USER_KEYRING
+# define KEY_SPEC_USER_KEYRING -4
+#endif
+
+#ifndef KEY_SPEC_USER_SESSION_KEYRING
+# define KEY_SPEC_USER_SESSION_KEYRING -5
+#endif
+
 #ifndef KEY_REQKEY_DEFL_THREAD_KEYRING
 # define KEY_REQKEY_DEFL_THREAD_KEYRING 1
 #endif
-- 
2.15.0.rc0.271.g36b669edcc-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [LTP] [PATCH v2 2/4] syscalls/keyctl06: new test for keyring_read() buffer overrun
  2017-10-12 20:42 [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs Eric Biggers
  2017-10-12 20:42 ` [LTP] [PATCH v2 1/4] lapi/keyctl.h: add a few missing definitions Eric Biggers
@ 2017-10-12 20:42 ` Eric Biggers
  2017-10-12 20:42 ` [LTP] [PATCH v2 3/4] syscalls/keyctl07: new test for oops when reading negative key Eric Biggers
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Eric Biggers @ 2017-10-12 20:42 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

Add a test for a bug which caused the kernel to write past the end of
the provided buffer when using KEYCTL_READ to read from a keyring.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/.gitignore        |  1 +
 testcases/kernel/syscalls/keyctl/keyctl06.c | 68 +++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+)
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl06.c

diff --git a/runtest/syscalls b/runtest/syscalls
index b1e988dce..fc6800abf 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -502,6 +502,7 @@ keyctl02 keyctl02
 keyctl03 keyctl03
 keyctl04 keyctl04
 keyctl05 keyctl05
+keyctl06 keyctl06
 
 kcmp01 kcmp01
 kcmp02 kcmp02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index 4dc2f8bfa..a803986ef 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -463,6 +463,7 @@
 /keyctl/keyctl03
 /keyctl/keyctl04
 /keyctl/keyctl05
+/keyctl/keyctl06
 /kcmp/kcmp01
 /kcmp/kcmp02
 /kcmp/kcmp03
diff --git a/testcases/kernel/syscalls/keyctl/keyctl06.c b/testcases/kernel/syscalls/keyctl/keyctl06.c
new file mode 100644
index 000000000..88734313d
--- /dev/null
+++ b/testcases/kernel/syscalls/keyctl/keyctl06.c
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2017 Google, Inc.
+ *
+ * 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 e645016abc80 ("KEYS: fix writing past end of
+ * user-supplied buffer in keyring_read()").
+ */
+
+#include <errno.h>
+
+#include "tst_test.h"
+#include "lapi/keyctl.h"
+
+static key_serial_t add_test_key(const char *description)
+{
+	TEST(add_key("user", description, "payload", 7,
+		     KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN < 0)
+		tst_brk(TBROK | TTERRNO, "Failed to add test key");
+	return TEST_RETURN;
+}
+
+static void do_test(void)
+{
+	key_serial_t key_ids[2];
+	key_serial_t key_id_1 = add_test_key("key1");
+	key_serial_t key_id_2 = add_test_key("key2");
+
+	memset(key_ids, 0, sizeof(key_ids));
+	TEST(keyctl(KEYCTL_READ, KEY_SPEC_PROCESS_KEYRING,
+		    (char *)key_ids, sizeof(key_serial_t)));
+	if (TEST_RETURN < 0)
+		tst_brk(TBROK | TTERRNO, "KEYCTL_READ failed");
+
+	if (key_ids[1] != 0)
+		tst_brk(TFAIL, "KEYCTL_READ overran the buffer");
+
+	if (key_ids[0] == 0)
+		tst_brk(TBROK, "KEYCTL_READ didn't read anything");
+
+	if (key_ids[0] != key_id_1 && key_ids[0] != key_id_2)
+		tst_brk(TBROK, "KEYCTL_READ didn't return correct key ID");
+
+	if (TEST_RETURN != sizeof(key_serial_t)) {
+		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
+			TEST_RETURN, sizeof(key_serial_t));
+	}
+
+	tst_res(TPASS, "KEYCTL_READ didn't overrun the buffer");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+};
-- 
2.15.0.rc0.271.g36b669edcc-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [LTP] [PATCH v2 3/4] syscalls/keyctl07: new test for oops when reading negative key
  2017-10-12 20:42 [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs Eric Biggers
  2017-10-12 20:42 ` [LTP] [PATCH v2 1/4] lapi/keyctl.h: add a few missing definitions Eric Biggers
  2017-10-12 20:42 ` [LTP] [PATCH v2 2/4] syscalls/keyctl06: new test for keyring_read() buffer overrun Eric Biggers
@ 2017-10-12 20:42 ` Eric Biggers
  2017-10-16 10:22   ` Cyril Hrubis
  2017-10-12 20:42 ` [LTP] [PATCH v2 4/4] syscalls/add_key03: new test for forging user keyrings Eric Biggers
  2017-10-13  7:25 ` [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs Petr Vorel
  4 siblings, 1 reply; 9+ messages in thread
From: Eric Biggers @ 2017-10-12 20:42 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

Add a test for a bug which caused the kernel to dereference a bogus
pointer when using KEYCTL_READ to read from a negative key.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/cve                                 |  1 +
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/.gitignore        |  1 +
 testcases/kernel/syscalls/keyctl/keyctl07.c | 89 +++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+)
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl07.c

diff --git a/runtest/cve b/runtest/cve
index d182e592a..b8e038112 100644
--- a/runtest/cve
+++ b/runtest/cve
@@ -17,5 +17,6 @@ cve-2017-2671 cve-2017-2671
 cve-2017-5669 cve-2017-5669
 cve-2017-6951 cve-2017-6951
 cve-2017-7472 keyctl04
+cve-2017-12192 keyctl07
 cve-2017-15274 add_key02
 cve-2017-1000364 stack_clash
diff --git a/runtest/syscalls b/runtest/syscalls
index fc6800abf..822608613 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -503,6 +503,7 @@ keyctl03 keyctl03
 keyctl04 keyctl04
 keyctl05 keyctl05
 keyctl06 keyctl06
+keyctl07 keyctl07
 
 kcmp01 kcmp01
 kcmp02 kcmp02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index a803986ef..8a2a8d21e 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -464,6 +464,7 @@
 /keyctl/keyctl04
 /keyctl/keyctl05
 /keyctl/keyctl06
+/keyctl/keyctl07
 /kcmp/kcmp01
 /kcmp/kcmp02
 /kcmp/kcmp03
diff --git a/testcases/kernel/syscalls/keyctl/keyctl07.c b/testcases/kernel/syscalls/keyctl/keyctl07.c
new file mode 100644
index 000000000..404e7a92f
--- /dev/null
+++ b/testcases/kernel/syscalls/keyctl/keyctl07.c
@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2017 Google, Inc.
+ *
+ * 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 37863c43b2c6 ("KEYS: prevent KEYCTL_READ on
+ * negative key").  This is CVE-2017-12192.
+ */
+
+#include <errno.h>
+
+#include "tst_test.h"
+#include "lapi/keyctl.h"
+
+static void do_test(void)
+{
+	key_serial_t key_id;
+	char buffer[128];
+
+	/*
+	 * Create a negatively instantiated key of the "user" key type.  This
+	 * key type is chosen because it has a ->read() method (which makes the
+	 * bug reachable) and is available whenever CONFIG_KEYS is enabled.
+	 *
+	 * request_key() will result in the creation of a negative key provided
+	 * that /sbin/request-key isn't configured to positively instantiate the
+	 * key, based on the provided type, description, and callout_info.  If
+	 * /sbin/request-key doesn't exist, errno will be ENOENT; while if it
+	 * does exist and we specify some random unprefixed description, errno
+	 * should be ENOKEY (since /sbin/request-key should not be configured to
+	 * instantiate random user keys).  In either case a negative key should
+	 * be created and we can continue on with the test.  Negative keys last
+	 * for 60 seconds so there should be plenty of time for the test.
+	 */
+	TEST(request_key("user", "description", "callout_info",
+			 KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN != -1)
+		tst_brk(TBROK, "request_key() unexpectedly succeeded");
+
+	if (TEST_ERRNO != ENOKEY && TEST_ERRNO != ENOENT) {
+		tst_brk(TBROK | TTERRNO,
+			"request_key() failed with unexpected error");
+	}
+
+	/* Get the ID of the negative key by reading the keyring */
+	TEST(keyctl(KEYCTL_READ, KEY_SPEC_PROCESS_KEYRING,
+		    &key_id, sizeof(key_id)));
+	if (TEST_RETURN < 0)
+		tst_brk(TBROK | TTERRNO, "KEYCTL_READ unexpectedly failed");
+	if (TEST_RETURN != sizeof(key_id)) {
+		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
+			TEST_RETURN, sizeof(key_id));
+	}
+
+	/*
+	 * Now try to read the negative key.  Unpatched kernels will oops trying
+	 * to read from memory address 0x00000000ffffff92.
+	 */
+	tst_res(TINFO, "trying to read from the negative key...");
+	TEST(keyctl(KEYCTL_READ, key_id, buffer, sizeof(buffer)));
+	if (TEST_RETURN != -1) {
+		tst_res(TFAIL,
+			"KEYCTL_READ on negative key unexpectedly succeeded");
+		return;
+	}
+	if (TEST_ERRNO != ENOKEY) {
+		tst_res(TFAIL | TTERRNO,
+			"KEYCTL_READ on negative key failed with unexpected error");
+		return;
+	}
+	tst_res(TPASS, "KEYCTL_READ on negative key expectedly failed with ENOKEY");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+};
-- 
2.15.0.rc0.271.g36b669edcc-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [LTP] [PATCH v2 4/4] syscalls/add_key03: new test for forging user keyrings
  2017-10-12 20:42 [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs Eric Biggers
                   ` (2 preceding siblings ...)
  2017-10-12 20:42 ` [LTP] [PATCH v2 3/4] syscalls/keyctl07: new test for oops when reading negative key Eric Biggers
@ 2017-10-12 20:42 ` Eric Biggers
  2017-10-13  7:25 ` [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs Petr Vorel
  4 siblings, 0 replies; 9+ messages in thread
From: Eric Biggers @ 2017-10-12 20:42 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

Add a test for a bug which allowed a user to create the user and user
session keyrings for another user.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/syscalls                              |  1 +
 testcases/kernel/syscalls/.gitignore          |  1 +
 testcases/kernel/syscalls/add_key/add_key03.c | 98 +++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)
 create mode 100644 testcases/kernel/syscalls/add_key/add_key03.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 822608613..e345325a2 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -13,6 +13,7 @@ acct01 acct01
 
 add_key01 add_key01
 add_key02 add_key02
+add_key03 add_key03
 
 adjtimex01 adjtimex01
 adjtimex02 adjtimex02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index 8a2a8d21e..2a770a998 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -8,6 +8,7 @@
 /acct/acct01
 /add_key/add_key01
 /add_key/add_key02
+/add_key/add_key03
 /adjtimex/adjtimex01
 /adjtimex/adjtimex02
 /alarm/alarm01
diff --git a/testcases/kernel/syscalls/add_key/add_key03.c b/testcases/kernel/syscalls/add_key/add_key03.c
new file mode 100644
index 000000000..27edcb27f
--- /dev/null
+++ b/testcases/kernel/syscalls/add_key/add_key03.c
@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2017 Google, Inc.
+ *
+ * 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 237bbd29f7a0 ("KEYS: prevent creating a different
+ * user's keyrings").  The bug allowed any random user to create a keyring named
+ * "_uid.$UID" (or "_uid_ses.$UID"), and it would become the user keyring (or
+ * user session keyring) for user $UID, provided that it hadn't already been
+ * created.
+ *
+ * This test must be run as root so that it has permission to switch to another
+ * user ID and check whether the keyrings are wrong.  However, the underlying
+ * bug is actually reachable/exploitable by a non-root user.
+ */
+
+#include <errno.h>
+#include <pwd.h>
+#include <stdio.h>
+
+#include "tst_test.h"
+#include "lapi/keyctl.h"
+
+static key_serial_t create_keyring(const char *description)
+{
+	TEST(add_key("keyring", description, NULL, 0,
+		     KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN < 0) {
+		tst_brk(TBROK | TTERRNO,
+			"unable to create keyring '%s'", description);
+	}
+	return TEST_RETURN;
+}
+
+static key_serial_t get_keyring_id(key_serial_t special_id)
+{
+	TEST(keyctl(KEYCTL_GET_KEYRING_ID, special_id, 1));
+	if (TEST_RETURN < 0) {
+		tst_brk(TBROK | TTERRNO,
+			"unable to get ID of keyring %d", special_id);
+	}
+	return TEST_RETURN;
+}
+
+static void do_test(void)
+{
+	uid_t uid = 1;
+	char description[32];
+	key_serial_t fake_user_keyring;
+	key_serial_t fake_user_session_keyring;
+
+	/*
+	 * We need a user to forge the keyrings for.  But the bug is not
+	 * reproducible for a UID which already has its keyrings, so find an
+	 * unused UID.  Note that it would be better to directly check for the
+	 * presence of the UID's keyrings than to search the passwd file.
+	 * However, that's not easy to do given that even if we assumed the UID
+	 * temporarily to check, KEYCTL_GET_KEYRING_ID for the user and user
+	 * session keyrings will create them rather than failing (even if the
+	 * 'create' argument is 0).
+	 */
+	while (getpwuid(uid))
+		uid++;
+
+	sprintf(description, "_uid.%u", uid);
+	fake_user_keyring = create_keyring(description);
+	sprintf(description, "_uid_ses.%u", uid);
+	fake_user_session_keyring = create_keyring(description);
+
+	SAFE_SETUID(uid);
+
+	if (fake_user_keyring == get_keyring_id(KEY_SPEC_USER_KEYRING))
+		tst_brk(TFAIL, "created user keyring for another user");
+
+	if (fake_user_session_keyring ==
+	    get_keyring_id(KEY_SPEC_USER_SESSION_KEYRING))
+		tst_brk(TFAIL, "created user session keyring for another user");
+
+	tst_res(TPASS, "expectedly could not create another user's keyrings");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_root = 1,
+};
-- 
2.15.0.rc0.271.g36b669edcc-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs
  2017-10-12 20:42 [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs Eric Biggers
                   ` (3 preceding siblings ...)
  2017-10-12 20:42 ` [LTP] [PATCH v2 4/4] syscalls/add_key03: new test for forging user keyrings Eric Biggers
@ 2017-10-13  7:25 ` Petr Vorel
  4 siblings, 0 replies; 9+ messages in thread
From: Petr Vorel @ 2017-10-13  7:25 UTC (permalink / raw)
  To: ltp

Hi Cyril,

> Eric Biggers (4):
>   lapi/keyctl.h: add a few missing definitions
>   syscalls/keyctl06: new test for keyring_read() buffer overrun
>   syscalls/keyctl07: new test for oops when reading negative key
>   syscalls/add_key03: new test for forging user keyrings

>  include/lapi/keyctl.h                         | 20 ++++++
>  runtest/cve                                   |  1 +
>  runtest/syscalls                              |  3 +
>  testcases/kernel/syscalls/.gitignore          |  3 +
>  testcases/kernel/syscalls/add_key/add_key03.c | 98 +++++++++++++++++++++++++++
>  testcases/kernel/syscalls/keyctl/keyctl06.c   | 68 +++++++++++++++++++
>  testcases/kernel/syscalls/keyctl/keyctl07.c   | 89 ++++++++++++++++++++++++
...
This is going to create conflict with my patch-set v3 "Fix 32-bit cross compilation".
If you accept this first, I can rebase my patch-set for you.
BTW my patch-set is in github, if it's easier for you.
https://github.com/pevik/ltp/tree/autotools/fix-32bit-build.v3


Kind regards,
Petr

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [LTP] [PATCH v2 3/4] syscalls/keyctl07: new test for oops when reading negative key
  2017-10-12 20:42 ` [LTP] [PATCH v2 3/4] syscalls/keyctl07: new test for oops when reading negative key Eric Biggers
@ 2017-10-16 10:22   ` Cyril Hrubis
  2017-10-16 19:47     ` Eric Biggers
  0 siblings, 1 reply; 9+ messages in thread
From: Cyril Hrubis @ 2017-10-16 10:22 UTC (permalink / raw)
  To: ltp

Hi!
I've pushed all patches but this one, since it actually looks like a
test bug when we oops the kernel since the test gets SIGKILL, which is
the same signal the library uses for a timeout:

 $ ./keyctl07
 tst_test.c:958: INFO: Timeout per run is 0h 05m 00s
 keyctl07.c:72: INFO: trying to read from the negative key...
 tst_test.c:1008: INFO: If you are running on slow machine, try
 exporting LTP_TIMEOUT_MUL > 1
 tst_test.c:1009: BROK: Test killed! (timeout?)

Can we, pretty please, run the test in a child process and wait it in
the test function and report a failure instead? I fear that kind of test
output will confuse people into thinking that this is a test problem
rather than a kernel bug.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [LTP] [PATCH v2 3/4] syscalls/keyctl07: new test for oops when reading negative key
  2017-10-16 10:22   ` Cyril Hrubis
@ 2017-10-16 19:47     ` Eric Biggers
  2017-10-17  9:19       ` Cyril Hrubis
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Biggers @ 2017-10-16 19:47 UTC (permalink / raw)
  To: ltp

On Mon, Oct 16, 2017 at 12:22:59PM +0200, Cyril Hrubis wrote:
> Hi!
> I've pushed all patches but this one, since it actually looks like a
> test bug when we oops the kernel since the test gets SIGKILL, which is
> the same signal the library uses for a timeout:
> 
>  $ ./keyctl07
>  tst_test.c:958: INFO: Timeout per run is 0h 05m 00s
>  keyctl07.c:72: INFO: trying to read from the negative key...
>  tst_test.c:1008: INFO: If you are running on slow machine, try
>  exporting LTP_TIMEOUT_MUL > 1
>  tst_test.c:1009: BROK: Test killed! (timeout?)
> 
> Can we, pretty please, run the test in a child process and wait it in
> the test function and report a failure instead? I fear that kind of test
> output will confuse people into thinking that this is a test problem
> rather than a kernel bug.
> 

Hi Cyril, I'll send out a new version that does that.  But please check that I'm
handling the exit status from the child process correctly because I wasn't 100%
sure how to do it.  (And I'm surprised more tests haven't run into this problem.
Maybe there should be something in the library for this, like being able to set
'.fail_on_sigkill = 1' in the 'struct tst_test'?  Or at least a different
version of tst_reap_children().)

Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [LTP] [PATCH v2 3/4] syscalls/keyctl07: new test for oops when reading negative key
  2017-10-16 19:47     ` Eric Biggers
@ 2017-10-17  9:19       ` Cyril Hrubis
  0 siblings, 0 replies; 9+ messages in thread
From: Cyril Hrubis @ 2017-10-17  9:19 UTC (permalink / raw)
  To: ltp

Hi!
> Hi Cyril, I'll send out a new version that does that.  But please check that I'm
> handling the exit status from the child process correctly because I wasn't 100%
> sure how to do it.  (And I'm surprised more tests haven't run into this problem.
> Maybe there should be something in the library for this, like being able to set
> '.fail_on_sigkill = 1' in the 'struct tst_test'?  Or at least a different
> version of tst_reap_children().)

Agree that the current state of the API for waiting child is indeed
suboptimal. The problem is that some tests should succeed on SIGKILL,
some fail, etc. So the interface would have to be quite flexible. Rather
than that I've decided to add new tst_strstatus() function so that we
can easily print what happened to the child. With that you can pick the
specific cases that should be mapped to TPASS and TFAIL first, then
handle the rest with TBROK.

Generally it's supposed to be used as:

	SAFE_WAIT(&status);

	if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
		tst_res(TPASS, ...);
		return;
	}

	if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL) {
		tst_res(TFAIL, ...);
		return;
	}

	tst_brk(TBROK, "Child ", tst_strstatus(status);


I guess that additional macros that would simplify the if conditions
would be helpful as well, something as TST_EXITEDWITH(status, 0) and
TST_SIGNALEDBY(status, SIGKILL).


We may as well try some kind of data driven approach with an array of
structures describing the exit-status to result mapping, howerever I
think that it would be an overkill in this case.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-10-17  9:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-12 20:42 [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs Eric Biggers
2017-10-12 20:42 ` [LTP] [PATCH v2 1/4] lapi/keyctl.h: add a few missing definitions Eric Biggers
2017-10-12 20:42 ` [LTP] [PATCH v2 2/4] syscalls/keyctl06: new test for keyring_read() buffer overrun Eric Biggers
2017-10-12 20:42 ` [LTP] [PATCH v2 3/4] syscalls/keyctl07: new test for oops when reading negative key Eric Biggers
2017-10-16 10:22   ` Cyril Hrubis
2017-10-16 19:47     ` Eric Biggers
2017-10-17  9:19       ` Cyril Hrubis
2017-10-12 20:42 ` [LTP] [PATCH v2 4/4] syscalls/add_key03: new test for forging user keyrings Eric Biggers
2017-10-13  7:25 ` [LTP] [PATCH v2 0/4] ltp: add tests for some recently-fixed keyrings bugs Petr Vorel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox