[PATCH 0/6] KEYS: Miscellaneous fixes

[PATCH 0/6] KEYS: Miscellaneous fixes

[David Howells]

Hi James,

Here are some fixes to go upstream:

 (1) Fix a bug in the assoc_array garbage collector [CVE-2014-3631].

 (2) Reinstate the production of EPERM for key types beginning with '.' in
     requests from userspace.

 (3) Fix some missing 'static' annotations.

 (4) Tidy up the cleanup of PKCS#7 message signed information blocks and fix a
     bug this made more obvious.

They can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-fixes

Tagged with:

	keys-fixes-20140910

David
---
David Howells (6):
      KEYS: Fix termination condition in assoc array garbage collection
      KEYS: Reinstate EPERM for a key type name beginning with a '.'
      KEYS: Fix missing statics
      PKCS#7: Add a missing static
      PKCS#7: Provide a single place to do signed info block freeing
      PKCS#7: Fix the parser cleanup to drain parsed out X.509 certs


 crypto/asymmetric_keys/pkcs7_parser.c |   61 ++++++++++++++++++---------------
 crypto/asymmetric_keys/pkcs7_trust.c  |    6 ++-
 lib/assoc_array.c                     |    4 ++
 security/keys/keyctl.c                |    2 +
 security/keys/request_key_auth.c      |    4 +-
 5 files changed, 43 insertions(+), 34 deletions(-)

[PATCH 1/6] KEYS: Fix termination condition in assoc array garbage collection

[David Howells]

This fixes CVE-2014-3631.

It is possible for an associative array to end up with a shortcut node at the
root of the tree if there are more than fan-out leaves in the tree, but they
all crowd into the same slot in the lowest level (ie. they all have the same
first nibble of their index keys).

When assoc_array_gc() returns back up the tree after scanning some leaves, it
can fall off of the root and crash because it assumes that the back pointer
from a shortcut (after label ascend_old_tree) must point to a normal node -
which isn't true of a shortcut node at the root.

Should we find we're ascending rootwards over a shortcut, we should check to
see if the backpointer is zero - and if it is, we have completed the scan.

This particular bug cannot occur if the root node is not a shortcut - ie. if
you have fewer than 17 keys in a keyring or if you have at least two keys that
sit into separate slots (eg. a keyring and a non keyring).

This can be reproduced by:

	ring=`keyctl newring bar @s`
	for ((i=1; i<=18; i++)); do last_key=`keyctl newring foo$i $ring`; done
	keyctl timeout $last_key 2

Doing this:

	echo 3 >/proc/sys/kernel/keys/gc_delay

first will speed things up.

If we do fall off of the top of the tree, we get the following oops:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
IP: [<ffffffff8136cea7>] assoc_array_gc+0x2f7/0x540
PGD dae15067 PUD cfc24067 PMD 0
Oops: 0000 [#1] SMP
Modules linked in: xt_nat xt_mark nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_rpfilter ip6t_REJECT xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_ni
CPU: 0 PID: 26011 Comm: kworker/0:1 Not tainted 3.14.9-200.fc20.x86_64 #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
Workqueue: events key_garbage_collector
task: ffff8800918bd580 ti: ffff8800aac14000 task.ti: ffff8800aac14000
RIP: 0010:[<ffffffff8136cea7>] [<ffffffff8136cea7>] assoc_array_gc+0x2f7/0x540
RSP: 0018:ffff8800aac15d40  EFLAGS: 00010206
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff8800aaecacc0
RDX: ffff8800daecf440 RSI: 0000000000000001 RDI: ffff8800aadc2bc0
RBP: ffff8800aac15da8 R08: 0000000000000001 R09: 0000000000000003
R10: ffffffff8136ccc7 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000070 R15: 0000000000000001
FS:  0000000000000000(0000) GS:ffff88011fc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000018 CR3: 00000000db10d000 CR4: 00000000000006f0
Stack:
 ffff8800aac15d50 0000000000000011 ffff8800aac15db8 ffffffff812e2a70
 ffff880091a00600 0000000000000000 ffff8800aadc2bc3 00000000cd42c987
 ffff88003702df20 ffff88003702dfa0 0000000053b65c09 ffff8800aac15fd8
Call Trace:
 [<ffffffff812e2a70>] ? keyring_detect_cycle_iterator+0x30/0x30
 [<ffffffff812e3e75>] keyring_gc+0x75/0x80
 [<ffffffff812e1424>] key_garbage_collector+0x154/0x3c0
 [<ffffffff810a67b6>] process_one_work+0x176/0x430
 [<ffffffff810a744b>] worker_thread+0x11b/0x3a0
 [<ffffffff810a7330>] ? rescuer_thread+0x3b0/0x3b0
 [<ffffffff810ae1a8>] kthread+0xd8/0xf0
 [<ffffffff810ae0d0>] ? insert_kthread_work+0x40/0x40
 [<ffffffff816ffb7c>] ret_from_fork+0x7c/0xb0
 [<ffffffff810ae0d0>] ? insert_kthread_work+0x40/0x40
Code: 08 4c 8b 22 0f 84 bf 00 00 00 41 83 c7 01 49 83 e4 fc 41 83 ff 0f 4c 89 65 c0 0f 8f 5a fe ff ff 48 8b 45 c0 4d 63 cf 49 83 c1 02 <4e> 8b 34 c8 4d 85 f6 0f 84 be 00 00 00 41 f6 c6 01 0f 84 92
RIP  [<ffffffff8136cea7>] assoc_array_gc+0x2f7/0x540
 RSP <ffff8800aac15d40>
CR2: 0000000000000018
---[ end trace 1129028a088c0cbd ]---

Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Don Zickus <dzickus@redhat.com>
---

 lib/assoc_array.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/assoc_array.c b/lib/assoc_array.c
index ae146f0734eb..2404d03e251a 100644
--- a/lib/assoc_array.c
+++ b/lib/assoc_array.c
@@ -1723,11 +1723,13 @@ ascend_old_tree:
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
 		slot = shortcut->parent_slot;
 		cursor = shortcut->back_pointer;
+		if (!cursor)
+			goto gc_complete;
 	} else {
 		slot = node->parent_slot;
 		cursor = ptr;
 	}
-	BUG_ON(!ptr);
+	BUG_ON(!cursor);
 	node = assoc_array_ptr_to_node(cursor);
 	slot++;
 	goto continue_node;

[PATCH 2/6] KEYS: Reinstate EPERM for a key type name beginning with a '.'

[David Howells]

Reinstate the generation of EPERM for a key type name beginning with a '.' in
a userspace call.  Types whose name begins with a '.' are internal only.

The test was removed by:

	commit a4e3b8d79a5c6d40f4a9703abf7fe3abcc6c3b8d
	Author: Mimi Zohar <zohar@linux.vnet.ibm.com>
	Date:   Thu May 22 14:02:23 2014 -0400
	Subject: KEYS: special dot prefixed keyring name bug fix

I think we want to keep the restriction on type name so that userspace can't
add keys of a special internal type.

Note that removal of the test causes several of the tests in the keyutils
testsuite to fail.

Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
---

 security/keys/keyctl.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index e26f860e5f2e..eff88a5f5d40 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -37,6 +37,8 @@ static int key_get_type_from_user(char *type,
 		return ret;
 	if (ret == 0 || ret >= len)
 		return -EINVAL;
+	if (type[0] == '.')
+		return -EPERM;
 	type[len - 1] = '\0';
 	return 0;
 }

Re: [PATCH 2/6] KEYS: Reinstate EPERM for a key type name beginning with a '.'

[Mimi Zohar]

On Wed, 2014-09-10 at 22:22 +0100, David Howells wrote: 
> Reinstate the generation of EPERM for a key type name beginning with a '.' in
> a userspace call.  Types whose name begins with a '.' are internal only.
> 
> The test was removed by:
> 
> 	commit a4e3b8d79a5c6d40f4a9703abf7fe3abcc6c3b8d
> 	Author: Mimi Zohar <zohar@linux.vnet.ibm.com>
> 	Date:   Thu May 22 14:02:23 2014 -0400
> 	Subject: KEYS: special dot prefixed keyring name bug fix
> 

The test was removed, as the patch log description indicates, because
the comparison is against the wrong field.  Even the function name
indicates key_get_type_from_user() is about the key type, not the key or
keyring name.

Debugging info:

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index e26f860..7a8d9b9 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -37,6 +37,7 @@ static int key_get_type_from_user(char *type,
                return ret;
        if (ret == 0 || ret >= len)
                return -EINVAL;
+       printk(KERN_INFO "%s: type %s \n", __func__, type);
        type[len - 1] = '\0';
        return 0;
}

: [    7.725429]: key_get_type_from_user: type trusted 
: [    9.007721] key_get_type_from_user: type keyring 
: [    9.011889] key_get_type_from_user: type keyring 
: [    9.068825] key_get_type_from_user: type encrypted 
: [    9.268310] key_get_type_from_user: type asymmetric 
: [    9.290360] key_get_type_from_user: type keyring 
: [    9.354509] key_get_type_from_user: type asymmetric 
: [    9.390396] key_get_type_from_user: type asymmetric 
: [    9.414216] key_get_type_from_user: type asymmetric 
: [    9.437155] key_get_type_from_user: type asymmetric 


> I think we want to keep the restriction on type name so that userspace can't
> add keys of a special internal type.

Definitely!  Commit a4e3b8d7 added a comparison against the description.

> Note that removal of the test causes several of the tests in the keyutils
> testsuite to fail.

Perhaps the added description test is insufficient, but reverting this
commit won't resolve the underlying problem.

Mimi

> Signed-off-by: David Howells <dhowells@redhat.com>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
> 
>  security/keys/keyctl.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index e26f860e5f2e..eff88a5f5d40 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -37,6 +37,8 @@ static int key_get_type_from_user(char *type,
>  		return ret;
>  	if (ret == 0 || ret >= len)
>  		return -EINVAL;
> +	if (type[0] == '.')
> +		return -EPERM;
>  	type[len - 1] = '\0';
>  	return 0;
>  }
> 

Re: [PATCH 2/6] KEYS: Reinstate EPERM for a key type name beginning with a '.'

[Mimi Zohar]

On Wed, 2014-09-10 at 19:36 -0400, Mimi Zohar wrote: 
> On Wed, 2014-09-10 at 22:22 +0100, David Howells wrote: 
> > Reinstate the generation of EPERM for a key type name beginning with a '.' in
> > a userspace call.  Types whose name begins with a '.' are internal only.

After re-reading your comment and looking at the different types,
testing for dot prefixed types now makes sense.  Both dot prefixed types
and keyring names are reserved for the kernel.

Mimi

Re: [PATCH 2/6] KEYS: Reinstate EPERM for a key type name beginning with a '.'

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> On Wed, 2014-09-10 at 19:36 -0400, Mimi Zohar wrote: 
> > On Wed, 2014-09-10 at 22:22 +0100, David Howells wrote: 
> > > Reinstate the generation of EPERM for a key type name beginning with a
> > > '.' in a userspace call.  Types whose name begins with a '.' are
> > > internal only.
> 
> After re-reading your comment and looking at the different types,
> testing for dot prefixed types now makes sense.  Both dot prefixed types
> and keyring names are reserved for the kernel.

Are you withdrawing your objection, then?

David

Re: [PATCH 2/6] KEYS: Reinstate EPERM for a key type name beginning with a '.'

[Dmitry Kasatkin]

On 11 September 2014 15:09, David Howells <dhowells@redhat.com> wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
>> On Wed, 2014-09-10 at 19:36 -0400, Mimi Zohar wrote:
>> > On Wed, 2014-09-10 at 22:22 +0100, David Howells wrote:
>> > > Reinstate the generation of EPERM for a key type name beginning with a
>> > > '.' in a userspace call.  Types whose name begins with a '.' are
>> > > internal only.
>>
>> After re-reading your comment and looking at the different types,
>> testing for dot prefixed types now makes sense.  Both dot prefixed types
>> and keyring names are reserved for the kernel.
>
> Are you withdrawing your objection, then?
>

For me, type test looks unrelated to "." prefixed key/keyring names...

The rest of that patch does following:

+ } else if ((description[0] == '.') &&
+                    (strncmp(type, "keyring", 7) == 0)) {
+             ret = -EPERM;
+             goto error2;


I wonder why this test is only disallowing keyrings...
Why not also keys?

keyctl add user ".ring1" Hello @u

keyctl show
  50463278 --alswrv      0     0       \_ user: .ring1


- Dmitry

> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html




-- 
Thanks,
Dmitry

Re: [PATCH 2/6] KEYS: Reinstate EPERM for a key type name beginning with a '.'

[Dmitry Kasatkin]

On 11 September 2014 15:27, Dmitry Kasatkin <dmitry.kasatkin@gmail.com> wrote:
> On 11 September 2014 15:09, David Howells <dhowells@redhat.com> wrote:
>> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>
>>> On Wed, 2014-09-10 at 19:36 -0400, Mimi Zohar wrote:
>>> > On Wed, 2014-09-10 at 22:22 +0100, David Howells wrote:
>>> > > Reinstate the generation of EPERM for a key type name beginning with a
>>> > > '.' in a userspace call.  Types whose name begins with a '.' are
>>> > > internal only.
>>>
>>> After re-reading your comment and looking at the different types,
>>> testing for dot prefixed types now makes sense.  Both dot prefixed types
>>> and keyring names are reserved for the kernel.
>>
>> Are you withdrawing your objection, then?
>>
>
> For me, type test looks unrelated to "." prefixed key/keyring names...
>
> The rest of that patch does following:
>
> + } else if ((description[0] == '.') &&
> +                    (strncmp(type, "keyring", 7) == 0)) {
> +             ret = -EPERM;
> +             goto error2;
>
>
> I wonder why this test is only disallowing keyrings...
> Why not also keys?
>
> keyctl add user ".ring1" Hello @u
>
> keyctl show
>   50463278 --alswrv      0     0       \_ user: .ring1
>
>

sorry... it was confusing name

keyctl newring ".ring1" @u
add_key: Operation not permitted

But for keys..

 keyctl add user ".key1" Hello @u

 keyctl show
   50463298 --alswrv      0     0       \_ user: .key1

- Dmitry

> - Dmitry
>
>> David
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
>
> --
> Thanks,
> Dmitry



-- 
Thanks,
Dmitry

Re: [PATCH 2/6] KEYS: Reinstate EPERM for a key type name beginning with a '.'

[Mimi Zohar]

On Thu, 2014-09-11 at 15:27 +0300, Dmitry Kasatkin wrote: 
> On 11 September 2014 15:09, David Howells <dhowells@redhat.com> wrote:
> > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >
> >> On Wed, 2014-09-10 at 19:36 -0400, Mimi Zohar wrote:
> >> > On Wed, 2014-09-10 at 22:22 +0100, David Howells wrote:
> >> > > Reinstate the generation of EPERM for a key type name beginning with a
> >> > > '.' in a userspace call.  Types whose name begins with a '.' are
> >> > > internal only.
> >>
> >> After re-reading your comment and looking at the different types,
> >> testing for dot prefixed types now makes sense.  Both dot prefixed types
> >> and keyring names are reserved for the kernel.
> >
> > Are you withdrawing your objection, then?

The misunderstanding was based on being told that dot prefixed keyrings
were protected, but in fact the only dot prefix code was here in
key_get_type_from_user().

The concept of dot prefixing should probably be documented either in the
code and/or documentation.  At this point, both types and keyring names
are reserved.

Mimi

Re: [PATCH 2/6] KEYS: Reinstate EPERM for a key type name beginning with a '.'

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> The concept of dot prefixing should probably be documented either in the
> code and/or documentation.  At this point, both types and keyring names
> are reserved.

Indeed.  There is one type whose name begins with a '.' that's sort of
internal to the kernel (".request_key_auth") that I definitely don't want
userspace to try creating.

The question of whether the description of a non-keyring key is permitted to
begin with a '.' is a separate issue to this patch.  We'd have to be sure that
no one is using keys of such a form.

David

[PATCH 3/6] KEYS: Fix missing statics

[David Howells]

Fix missing statics (found by checker).

Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
---

 security/keys/request_key_auth.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 842e6f410d50..739e7455d388 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -44,12 +44,12 @@ struct key_type key_type_request_key_auth = {
 	.read		= request_key_auth_read,
 };
 
-int request_key_auth_preparse(struct key_preparsed_payload *prep)
+static int request_key_auth_preparse(struct key_preparsed_payload *prep)
 {
 	return 0;
 }
 
-void request_key_auth_free_preparse(struct key_preparsed_payload *prep)
+static void request_key_auth_free_preparse(struct key_preparsed_payload *prep)
 {
 }
 

[PATCH 4/6] PKCS#7: Add a missing static

[David Howells]

Add a missing static (found by checker).

Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
---

 crypto/asymmetric_keys/pkcs7_trust.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c
index e666eb011a85..fad888ea4fad 100644
--- a/crypto/asymmetric_keys/pkcs7_trust.c
+++ b/crypto/asymmetric_keys/pkcs7_trust.c
@@ -23,9 +23,9 @@
 /**
  * Check the trust on one PKCS#7 SignedInfo block.
  */
-int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
-			     struct pkcs7_signed_info *sinfo,
-			     struct key *trust_keyring)
+static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
+				    struct pkcs7_signed_info *sinfo,
+				    struct key *trust_keyring)
 {
 	struct public_key_signature *sig = &sinfo->sig;
 	struct x509_certificate *x509, *last = NULL, *p;

[PATCH 5/6] PKCS#7: Provide a single place to do signed info block freeing

[David Howells]

The code to free a signed info block is repeated several times, so move the
code to do it into a function of its own.  This gives us a place to add clean
ups for stuff that gets added to pkcs7_signed_info.

Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
---

 crypto/asymmetric_keys/pkcs7_parser.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
index 42e56aa7d277..4c4ea35c338b 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -31,6 +31,18 @@ struct pkcs7_parse_context {
 	unsigned	sinfo_index;
 };
 
+/*
+ * Free a signed information block.
+ */
+static void pkcs7_free_signed_info(struct pkcs7_signed_info *sinfo)
+{
+	if (sinfo) {
+		mpi_free(sinfo->sig.mpi[0]);
+		kfree(sinfo->sig.digest);
+		kfree(sinfo);
+	}
+}
+
 /**
  * pkcs7_free_message - Free a PKCS#7 message
  * @pkcs7: The PKCS#7 message to free
@@ -54,9 +66,7 @@ void pkcs7_free_message(struct pkcs7_message *pkcs7)
 		while (pkcs7->signed_infos) {
 			sinfo = pkcs7->signed_infos;
 			pkcs7->signed_infos = sinfo->next;
-			mpi_free(sinfo->sig.mpi[0]);
-			kfree(sinfo->sig.digest);
-			kfree(sinfo);
+			pkcs7_free_signed_info(sinfo);
 		}
 		kfree(pkcs7);
 	}
@@ -100,16 +110,12 @@ struct pkcs7_message *pkcs7_parse_message(const void *data, size_t datalen)
 		ctx->certs = cert->next;
 		x509_free_certificate(cert);
 	}
-	mpi_free(ctx->sinfo->sig.mpi[0]);
-	kfree(ctx->sinfo->sig.digest);
-	kfree(ctx->sinfo);
+	pkcs7_free_signed_info(ctx->sinfo);
 	kfree(ctx);
 	return msg;
 
 error_decode:
-	mpi_free(ctx->sinfo->sig.mpi[0]);
-	kfree(ctx->sinfo->sig.digest);
-	kfree(ctx->sinfo);
+	pkcs7_free_signed_info(ctx->sinfo);
 error_no_sinfo:
 	kfree(ctx);
 error_no_ctx:

[PATCH 6/6] PKCS#7: Fix the parser cleanup to drain parsed out X.509 certs

[David Howells]

Fix the parser cleanup code to drain parsed out X.509 certs in the case that
the decode fails and we jump to error_decode.

The function is rearranged so that the same cleanup code is used in the success
case as the error case - just that the message descriptor under construction is
only released if it is still pointed to by the context struct at that point.

Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
---

 crypto/asymmetric_keys/pkcs7_parser.c |   39 ++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
index 4c4ea35c338b..1e9861da7ee4 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -81,47 +81,46 @@ EXPORT_SYMBOL_GPL(pkcs7_free_message);
 struct pkcs7_message *pkcs7_parse_message(const void *data, size_t datalen)
 {
 	struct pkcs7_parse_context *ctx;
-	struct pkcs7_message *msg;
-	long ret;
+	struct pkcs7_message *msg = ERR_PTR(-ENOMEM);
+	int ret;
 
-	ret = -ENOMEM;
-	msg = kzalloc(sizeof(struct pkcs7_message), GFP_KERNEL);
-	if (!msg)
-		goto error_no_sig;
 	ctx = kzalloc(sizeof(struct pkcs7_parse_context), GFP_KERNEL);
 	if (!ctx)
-		goto error_no_ctx;
+		goto out_no_ctx;
+	ctx->msg = kzalloc(sizeof(struct pkcs7_message), GFP_KERNEL);
+	if (!ctx->msg)
+		goto out_no_msg;
 	ctx->sinfo = kzalloc(sizeof(struct pkcs7_signed_info), GFP_KERNEL);
 	if (!ctx->sinfo)
-		goto error_no_sinfo;
+		goto out_no_sinfo;
 
-	ctx->msg = msg;
 	ctx->data = (unsigned long)data;
 	ctx->ppcerts = &ctx->certs;
 	ctx->ppsinfo = &ctx->msg->signed_infos;
 
 	/* Attempt to decode the signature */
 	ret = asn1_ber_decoder(&pkcs7_decoder, ctx, data, datalen);
-	if (ret < 0)
-		goto error_decode;
+	if (ret < 0) {
+		msg = ERR_PTR(ret);
+		goto out;
+	}
+
+	msg = ctx->msg;
+	ctx->msg = NULL;
 
+out:
 	while (ctx->certs) {
 		struct x509_certificate *cert = ctx->certs;
 		ctx->certs = cert->next;
 		x509_free_certificate(cert);
 	}
 	pkcs7_free_signed_info(ctx->sinfo);
+out_no_sinfo:
+	pkcs7_free_message(ctx->msg);
+out_no_msg:
 	kfree(ctx);
+out_no_ctx:
 	return msg;
-
-error_decode:
-	pkcs7_free_signed_info(ctx->sinfo);
-error_no_sinfo:
-	kfree(ctx);
-error_no_ctx:
-	pkcs7_free_message(msg);
-error_no_sig:
-	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(pkcs7_parse_message);
 

Re: [PATCH 0/6] KEYS: Miscellaneous fixes

[David Howells]

Hi James,

Thanks for pushing the first patch.  Any thoughts on what to do with the
remaining patches?  #5 and #6 at least ought to go upstream as there's a
potential memory leak fixed and #2 because the keyutils testsuite is failing
without it.  From what Mimi says, I think she's withdrawn her objection to #2.

Thanks,
David

Re: [PATCH 0/6] KEYS: Miscellaneous fixes

[James Morris]

On Mon, 15 Sep 2014, David Howells wrote:

> Hi James,
> 
> Thanks for pushing the first patch.  Any thoughts on what to do with the
> remaining patches?  #5 and #6 at least ought to go upstream as there's a
> potential memory leak fixed and #2 because the keyutils testsuite is failing
> without it.  From what Mimi says, I think she's withdrawn her objection to #2.

Can you split your pull requests into different branches -- one which is 
for current upstream, and one which is for next?


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 0/6] KEYS: Miscellaneous fixes

[David Howells]

James Morris <jmorris@namei.org> wrote:

> Can you split your pull requests into different branches -- one which is 
> for current upstream, and one which is for next?

I had already done that.  But if you really want, I can punt #3 and #4 over to
linux-next.

David