[LTP] [PATCH 0/2] ltp: update add_key tests for nonempty NULL payload fix

[LTP] [PATCH 0/2] ltp: update add_key tests for nonempty NULL payload fix

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

These two patches remove the existing add_key02 test, which is having its
behavior changed, and replaces it with the more thorough add_key03 test which
specifically tests the "NULL payload with nonzero length" case, which was
subject to a kernel bug causing a NULL dereference with some key types.

(The reason I'm not simply changing the add_key02 test is that I don't want it
to appear as a regression on old kernels.  I'm not sure exactly how this kind of
thing is typically handled in LTP, though.)

Eric Biggers (2):
  syscalls/add_key02: remove test
  syscalls/add_key03: add test for NULL payload with nonzero length

 runtest/syscalls                              |   2 +-
 testcases/kernel/syscalls/.gitignore          |   2 +-
 testcases/kernel/syscalls/add_key/add_key02.c |  74 ------------------
 testcases/kernel/syscalls/add_key/add_key03.c | 104 ++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 76 deletions(-)
 delete mode 100644 testcases/kernel/syscalls/add_key/add_key02.c
 create mode 100644 testcases/kernel/syscalls/add_key/add_key03.c

-- 
2.13.0.506.g27d5fe0cd-goog

[LTP] [PATCH 1/2] syscalls/add_key02: remove test

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

add_key02 was supposed to be a "Basic test for the add_key() syscall",
but it actually happened to test the obscure case of passing a NULL
payload with nonzero length.  This case was mishandled by the kernel,
which either returned EINVAL or crashed with a NULL pointer dereference,
depending on the key type.  (The former applied to the test, as it used
the "user" key type.)  The expected behavior is that the syscall fail
with EFAULT.

As this is being fixed by the kernel patch "KEYS: fix dereferencing NULL
payload with nonzero length", the test needed to be updated.  To avoid
making it appear as a regression, just remove the test; the next patch
will add a replacement test as add_key03.  The new test will also be
more thorough as it will test other key types besides "user", including
ones that cause a NULL pointer dereference in unfixed kernels.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/syscalls                              |  1 -
 testcases/kernel/syscalls/.gitignore          |  1 -
 testcases/kernel/syscalls/add_key/add_key02.c | 74 ---------------------------
 3 files changed, 76 deletions(-)
 delete mode 100644 testcases/kernel/syscalls/add_key/add_key02.c

diff --git a/runtest/syscalls b/runtest/syscalls
index fe5227290..0c3c46e57 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -12,7 +12,6 @@ access04 access04
 acct01 acct01
 
 add_key01 add_key01
-add_key02 add_key02
 
 adjtimex01 adjtimex01
 adjtimex02 adjtimex02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index c14c4e66b..5b8df06f5 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -7,7 +7,6 @@
 /access/access04
 /acct/acct01
 /add_key/add_key01
-/add_key/add_key02
 /adjtimex/adjtimex01
 /adjtimex/adjtimex02
 /alarm/alarm01
diff --git a/testcases/kernel/syscalls/add_key/add_key02.c b/testcases/kernel/syscalls/add_key/add_key02.c
deleted file mode 100644
index 866800d6f..000000000
--- a/testcases/kernel/syscalls/add_key/add_key02.c
+++ /dev/null
@@ -1,74 +0,0 @@
-/******************************************************************************
- * Copyright (c) Crackerjack Project., 2007				      *
- *									      *
- * 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, write to the Free Software	Foundation,   *
- * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA           *
- *									      *
- ******************************************************************************/
-
-/*
- * Basic test for the add_key() syscall.
- *
- * History:   Porting from Crackerjack to LTP is done by
- *	      Manas Kumar Nayak maknayak@in.ibm.com>
- */
-
-#include "config.h"
-#ifdef HAVE_LINUX_KEYCTL_H
-# include <linux/keyctl.h>
-#endif
-#include "tst_test.h"
-#include "linux_syscall_numbers.h"
-
-#ifdef HAVE_LINUX_KEYCTL_H
-struct tcase {
-	char *type;
-	char *desc;
-	void *payload;
-	int plen;
-	int exp_errno;
-} tcases[] = {
-	{"user", "firstkey", NULL, 1, EINVAL}
-};
-#endif /* HAVE_LINUX_KEYCTL_H */
-
-static void verify_add_key(unsigned int i)
-{
-#ifdef HAVE_LINUX_KEYCTL_H
-	TEST(tst_syscall(__NR_add_key, tcases[i].type, tcases[i].desc,
-	                 tcases[i].payload, tcases[i].plen,
-	                 KEY_SPEC_USER_KEYRING));
-
-	if (TEST_RETURN != -1) {
-		tst_res(TFAIL, "add_key() passed unexpectedly");
-		return;
-	}
-
-	if (TEST_ERRNO == tcases[i].exp_errno) {
-		tst_res(TPASS | TTERRNO, "add_key() failed expectedly");
-		return;
-	}
-
-	tst_res(TFAIL | TTERRNO,
-	        "add_key() failed unexpectedly, expected %s",
-	        tst_strerrno(tcases[i].exp_errno));
-#else
-	tst_brk(TCONF, "linux/keyctl.h was missing upon compilation.");
-#endif /* HAVE_LINUX_KEYCTL_H */
-}
-
-static struct tst_test test = {
-	.tcnt = ARRAY_SIZE(tcases),
-	.test = verify_add_key,
-};
-- 
2.13.0.506.g27d5fe0cd-goog

[LTP] [PATCH 2/2] syscalls/add_key03: add test for NULL payload with nonzero length

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Add a new test program to test that the add_key() syscall correctly
handles a NULL payload with nonzero length.  Note that may cause a NULL
pointer dereference in unpatched kernels.

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

diff --git a/runtest/syscalls b/runtest/syscalls
index 0c3c46e57..618089801 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -12,6 +12,7 @@ access04 access04
 acct01 acct01
 
 add_key01 add_key01
+add_key03 add_key03
 
 adjtimex01 adjtimex01
 adjtimex02 adjtimex02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index 5b8df06f5..b5b428df5 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -7,6 +7,7 @@
 /access/access04
 /acct/acct01
 /add_key/add_key01
+/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..21812710f
--- /dev/null
+++ b/testcases/kernel/syscalls/add_key/add_key03.c
@@ -0,0 +1,104 @@
+/*
+ * 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/>.
+ */
+
+#include "config.h"
+#ifdef HAVE_LINUX_KEYCTL_H
+# include <linux/keyctl.h>
+#endif
+#include "tst_test.h"
+#include "linux_syscall_numbers.h"
+
+/*
+ * Test that the add_key() syscall correctly handles a NULL payload with nonzero
+ * length.  Specifically, it should fail with EFAULT rather than oopsing the
+ * kernel with a NULL pointer dereference or failing with EINVAL, as it did
+ * before (depending on the key type).  This is a regression test for "KEYS: fix
+ * dereferencing NULL payload with nonzero length".
+ *
+ * Note that none of the key types that exhibited the NULL pointer dereference
+ * are guaranteed to be built into the kernel, so we just test as many as we
+ * can, in the hope of catching one.  We also test with the "user" key type for
+ * good measure, although it was one of the types that failed with EINVAL rather
+ * than dereferencing NULL.
+ */
+
+#ifdef HAVE_LINUX_KEYCTL_H
+struct tcase {
+	const char *type;
+	size_t plen;
+} tcases[] = {
+	/*
+	 * The payload length we test for each key type needs to pass initial
+	 * validation but is otherwise arbitrary.  Note: the "rxrpc_s" key type
+	 * requires a payload of exactly 8 bytes.
+	 */
+	{ "asymmetric",		64 },
+	{ "cifs.idmap",		64 },
+	{ "cifs.spnego",	64 },
+	{ "pkcs7_test",		64 },
+	{ "rxrpc",		64 },
+	{ "rxrpc_s",		 8 },
+	{ "user",		64 },
+};
+#endif /* HAVE_LINUX_KEYCTL_H */
+
+static void verify_add_key(unsigned int i)
+{
+#ifdef HAVE_LINUX_KEYCTL_H
+	TEST(tst_syscall(__NR_add_key, tcases[i].type, "abc:def",
+			 NULL, tcases[i].plen, KEY_SPEC_PROCESS_KEYRING));
+
+	if (TEST_RETURN != -1) {
+		tst_res(TFAIL,
+			"add_key() with key type \"%s\" unexpectedly succeeded",
+			tcases[i].type);
+		return;
+	}
+
+	if (TEST_ERRNO == EFAULT) {
+		tst_res(TPASS, "received expected EFAULT with key type \"%s\"",
+			tcases[i].type);
+		return;
+	}
+
+	if (TEST_ERRNO == ENODEV) {
+		tst_res(TCONF, "kernel doesn't support key type \"%s\"",
+			tcases[i].type);
+		return;
+	}
+
+	/*
+	 * It's possible for the "asymmetric" key type to be supported, but with
+	 * no asymmetric key parsers registered.  In that case, attempting to
+	 * add a key of type asymmetric will fail with EBADMSG.
+	 */
+	if (TEST_ERRNO == EBADMSG && !strcmp(tcases[i].type, "asymmetric")) {
+		tst_res(TCONF, "no asymmetric key parsers are registered");
+		return;
+	}
+
+	tst_res(TFAIL | TTERRNO, "unexpected error with key type \"%s\"",
+		tcases[i].type);
+#else
+	tst_brk(TCONF, "linux/keyctl.h was missing upon compilation.");
+#endif /* HAVE_LINUX_KEYCTL_H */
+}
+
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = verify_add_key,
+};
-- 
2.13.0.506.g27d5fe0cd-goog

[LTP] [PATCH 2/2] syscalls/add_key03: add test for NULL payload with nonzero length

[Cyril Hrubis]

Hi!
> 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..21812710f
> --- /dev/null
> +++ b/testcases/kernel/syscalls/add_key/add_key03.c
> @@ -0,0 +1,104 @@
> +/*
> + * 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/>.
> + */
> +
> +#include "config.h"
> +#ifdef HAVE_LINUX_KEYCTL_H
> +# include <linux/keyctl.h>
> +#endif
> +#include "tst_test.h"
> +#include "linux_syscall_numbers.h"
> +
> +/*
> + * Test that the add_key() syscall correctly handles a NULL payload with nonzero
> + * length.  Specifically, it should fail with EFAULT rather than oopsing the
> + * kernel with a NULL pointer dereference or failing with EINVAL, as it did
> + * before (depending on the key type).  This is a regression test for "KEYS: fix
> + * dereferencing NULL payload with nonzero length".

Can you pretty please add the kernel commit hash here as well?

> + * Note that none of the key types that exhibited the NULL pointer dereference
> + * are guaranteed to be built into the kernel, so we just test as many as we
> + * can, in the hope of catching one.  We also test with the "user" key type for
> + * good measure, although it was one of the types that failed with EINVAL rather
> + * than dereferencing NULL.
> + */
> +
> +#ifdef HAVE_LINUX_KEYCTL_H
> +struct tcase {
> +	const char *type;
> +	size_t plen;
> +} tcases[] = {
> +	/*
> +	 * The payload length we test for each key type needs to pass initial
> +	 * validation but is otherwise arbitrary.  Note: the "rxrpc_s" key type
> +	 * requires a payload of exactly 8 bytes.
> +	 */
> +	{ "asymmetric",		64 },
> +	{ "cifs.idmap",		64 },
> +	{ "cifs.spnego",	64 },
> +	{ "pkcs7_test",		64 },
> +	{ "rxrpc",		64 },
> +	{ "rxrpc_s",		 8 },
> +	{ "user",		64 },
> +};
> +#endif /* HAVE_LINUX_KEYCTL_H */
> +
> +static void verify_add_key(unsigned int i)
> +{
> +#ifdef HAVE_LINUX_KEYCTL_H
> +	TEST(tst_syscall(__NR_add_key, tcases[i].type, "abc:def",
> +			 NULL, tcases[i].plen, KEY_SPEC_PROCESS_KEYRING));
> +
> +	if (TEST_RETURN != -1) {
> +		tst_res(TFAIL,
> +			"add_key() with key type \"%s\" unexpectedly succeeded",
                                                  ^
						  And we tend to use
						  single quotes that
						  does not have to be
						  escaped. But that is
						  very minor.

Otherwise the test looks good to me.


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] syscalls/add_key03: add test for NULL payload with nonzero length

[Eric Biggers]

On Tue, Jun 06, 2017 at 02:06:56PM +0200, Cyril Hrubis wrote:
> Hi!
> > 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..21812710f
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/add_key/add_key03.c
> > @@ -0,0 +1,104 @@
> > +/*
> > + * 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/>.
> > + */
> > +
> > +#include "config.h"
> > +#ifdef HAVE_LINUX_KEYCTL_H
> > +# include <linux/keyctl.h>
> > +#endif
> > +#include "tst_test.h"
> > +#include "linux_syscall_numbers.h"
> > +
> > +/*
> > + * Test that the add_key() syscall correctly handles a NULL payload with nonzero
> > + * length.  Specifically, it should fail with EFAULT rather than oopsing the
> > + * kernel with a NULL pointer dereference or failing with EINVAL, as it did
> > + * before (depending on the key type).  This is a regression test for "KEYS: fix
> > + * dereferencing NULL payload with nonzero length".
> 
> Can you pretty please add the kernel commit hash here as well?
> 

It's not in mainline yet, but I'll resend once it is.

> > +		tst_res(TFAIL,
> > +			"add_key() with key type \"%s\" unexpectedly succeeded",
>                                                   ^
> 						  And we tend to use
> 						  single quotes that
> 						  does not have to be
> 						  escaped. But that is
> 						  very minor.

Will do in v2.

Thanks,

- Eric

[LTP] [PATCH 0/2] ltp: update add_key tests for nonempty NULL payload fix

[Cyril Hrubis]

Hi!
> (The reason I'm not simply changing the add_key02 test is that I don't want it
> to appear as a regression on old kernels.  I'm not sure exactly how this kind of
> thing is typically handled in LTP, though.)

What exactly do you mean by a regression here? The fact that it may
crash older kernels? That is pretty much fine, when test fails/kernel
crashes manual intervention is required and one of the first things you
do is git log for the test.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 0/2] ltp: update add_key tests for nonempty NULL payload fix

[Eric Biggers]

Hi Cyril,

On Tue, Jun 06, 2017 at 01:55:52PM +0200, Cyril Hrubis wrote:
> Hi!
> > (The reason I'm not simply changing the add_key02 test is that I don't want it
> > to appear as a regression on old kernels.  I'm not sure exactly how this kind of
> > thing is typically handled in LTP, though.)
> 
> What exactly do you mean by a regression here? The fact that it may
> crash older kernels? That is pretty much fine, when test fails/kernel
> crashes manual intervention is required and one of the first things you
> do is git log for the test.
> 

When I had tried updating existing tests in xfstests to test more cases, I was
told it shouldn't be done because then the test would start failing on some
systems as a result of the test change, rather than as a result of a kernel
regression.  So it would be more difficult to figure out what caused the test
regression.

Personally, I don't align quite as strongly with that viewpoint (and maybe you
and the other LTP developers don't either) because this can, after all, only be
a problem in cases where the test suite is upgraded, and furthermore if a test
fails it's helpful to 'git log' the test anyway.

Would you suggest I simply update add_key02 instead?

Eric

[LTP] [PATCH 0/2] ltp: update add_key tests for nonempty NULL payload fix

[Cyril Hrubis]

Hi!
> Personally, I don't align quite as strongly with that viewpoint (and maybe you
> and the other LTP developers don't either) because this can, after all, only be
> a problem in cases where the test suite is upgraded, and furthermore if a test
> fails it's helpful to 'git log' the test anyway.
> 
> Would you suggest I simply update add_key02 instead?

Yes, I would have done that as well.

-- 
Cyril Hrubis
chrubis@suse.cz