diff mbox

KEYS: Ensure expired keys are renewed

Message ID 20141030174612.10093.61557.stgit@manet.1015granger.net (mailing list archive)
State New, archived
Headers show

Commit Message

Chuck Lever III Oct. 30, 2014, 5:46 p.m. UTC 
After about 10 minutes, my NFSv4 functional tests fail because the
ownership of the test files goes to "-2". Looking at /proc/keys
shows that the id_resolv keys that map to my test user ID have
expired. The ownership problem persists until the expired keys are
purged from the keyring, and fresh keys are obtained.

I bisected the problem to 3.13 commit b2a4df200d57 ("KEYS: Expand
the capacity of a keyring"). This commit inadvertantly changes the
API contract of the internal function keyring_search_aux().

The root cause appears to be that b2a4df200d57 made "no state check"
the default behavior. "No state check" means the keyring search
iterator function skips checking the key's expiry timeout, and
returns expired keys.  request_key_and_link() depends on getting
an -EAGAIN result code to know when to perform an upcall to refresh
an expired key.

Restore the previous default behavior of keyring_search_aux() and
the meaning of the KEYRING_SEARCH_{DO,NO}_STATE_CHECK flags.

Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
Resend with correct linux-nfs address.

 security/keys/keyring.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)


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

Comments

NeilBrown Nov. 13, 2014, 12:15 a.m. UTC | #1 
On Thu, 30 Oct 2014 13:46:47 -0400 Chuck Lever <chuck.lever@oracle.com> wrote:

> After about 10 minutes, my NFSv4 functional tests fail because the
> ownership of the test files goes to "-2". Looking at /proc/keys
> shows that the id_resolv keys that map to my test user ID have
> expired. The ownership problem persists until the expired keys are
> purged from the keyring, and fresh keys are obtained.
> 
> I bisected the problem to 3.13 commit b2a4df200d57 ("KEYS: Expand
> the capacity of a keyring"). This commit inadvertantly changes the
> API contract of the internal function keyring_search_aux().
> 
> The root cause appears to be that b2a4df200d57 made "no state check"
> the default behavior. "No state check" means the keyring search
> iterator function skips checking the key's expiry timeout, and
> returns expired keys.  request_key_and_link() depends on getting
> an -EAGAIN result code to know when to perform an upcall to refresh
> an expired key.
> 
> Restore the previous default behavior of keyring_search_aux() and
> the meaning of the KEYRING_SEARCH_{DO,NO}_STATE_CHECK flags.
> 
> Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> Resend with correct linux-nfs address.
> 
>  security/keys/keyring.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 8314a7d..1294582 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -519,9 +519,14 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
>  	struct keyring_search_context *ctx = iterator_data;
>  	const struct key *key = keyring_ptr_to_key(object);
>  	unsigned long kflags = key->flags;
> +	bool state_check_needed;
>  
>  	kenter("{%d}", key->serial);
>  
> +	state_check_needed = !(ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK);
> +	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK)
> +		state_check_needed = true;
> +
>  	/* ignore keys not of this type */
>  	if (key->type != ctx->index_key.type) {
>  		kleave(" = 0 [!type]");
> @@ -529,7 +534,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
>  	}
>  
>  	/* skip invalidated, revoked and expired keys */
> -	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
> +	if (state_check_needed) {
>  		if (kflags & ((1 << KEY_FLAG_INVALIDATED) |
>  			      (1 << KEY_FLAG_REVOKED))) {
>  			ctx->result = ERR_PTR(-EKEYREVOKED);
> @@ -559,7 +564,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
>  		goto skipped;
>  	}
>  
> -	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
> +	if (state_check_needed) {
>  		/* we set a different error code if we pass a negative key */
>  		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
>  			smp_rmb();
> @@ -642,8 +647,6 @@ static bool search_nested_keyrings(struct key *keyring,
>  	}
>  
>  	ctx->skipped_ret = 0;
> -	if (ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK)
> -		ctx->flags &= ~KEYRING_SEARCH_DO_STATE_CHECK;
>  
>  	/* Start processing a new keyring */
>  descend_to_keyring:
> 

Reviewed-by: NeilBrown <neilb@suse.de>

security/keys/internal.h says:

#define KEYRING_SEARCH_NO_STATE_CHECK   0x0001  /* Skip state checks */
#define KEYRING_SEARCH_DO_STATE_CHECK   0x0002  /* Override NO_STATE_CHECK */


Your match makes it obvious that DO overrides NO.  The current code
doesn't get that right.

Is this on its way upstream yet (not in -next that I could see).

Thanks,
NeilBrown
Chuck Lever III Nov. 13, 2014, 3:09 p.m. UTC | #2 
On Nov 12, 2014, at 6:15 PM, NeilBrown <neilb@suse.de> wrote:

> On Thu, 30 Oct 2014 13:46:47 -0400 Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> After about 10 minutes, my NFSv4 functional tests fail because the
>> ownership of the test files goes to "-2". Looking at /proc/keys
>> shows that the id_resolv keys that map to my test user ID have
>> expired. The ownership problem persists until the expired keys are
>> purged from the keyring, and fresh keys are obtained.
>> 
>> I bisected the problem to 3.13 commit b2a4df200d57 ("KEYS: Expand
>> the capacity of a keyring"). This commit inadvertantly changes the
>> API contract of the internal function keyring_search_aux().
>> 
>> The root cause appears to be that b2a4df200d57 made "no state check"
>> the default behavior. "No state check" means the keyring search
>> iterator function skips checking the key's expiry timeout, and
>> returns expired keys.  request_key_and_link() depends on getting
>> an -EAGAIN result code to know when to perform an upcall to refresh
>> an expired key.
>> 
>> Restore the previous default behavior of keyring_search_aux() and
>> the meaning of the KEYRING_SEARCH_{DO,NO}_STATE_CHECK flags.
>> 
>> Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> Resend with correct linux-nfs address.
>> 
>> security/keys/keyring.c |   11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>> 
>> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
>> index 8314a7d..1294582 100644
>> --- a/security/keys/keyring.c
>> +++ b/security/keys/keyring.c
>> @@ -519,9 +519,14 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
>> 	struct keyring_search_context *ctx = iterator_data;
>> 	const struct key *key = keyring_ptr_to_key(object);
>> 	unsigned long kflags = key->flags;
>> +	bool state_check_needed;
>> 
>> 	kenter("{%d}", key->serial);
>> 
>> +	state_check_needed = !(ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK);
>> +	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK)
>> +		state_check_needed = true;
>> +
>> 	/* ignore keys not of this type */
>> 	if (key->type != ctx->index_key.type) {
>> 		kleave(" = 0 [!type]");
>> @@ -529,7 +534,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
>> 	}
>> 
>> 	/* skip invalidated, revoked and expired keys */
>> -	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
>> +	if (state_check_needed) {
>> 		if (kflags & ((1 << KEY_FLAG_INVALIDATED) |
>> 			      (1 << KEY_FLAG_REVOKED))) {
>> 			ctx->result = ERR_PTR(-EKEYREVOKED);
>> @@ -559,7 +564,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
>> 		goto skipped;
>> 	}
>> 
>> -	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
>> +	if (state_check_needed) {
>> 		/* we set a different error code if we pass a negative key */
>> 		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
>> 			smp_rmb();
>> @@ -642,8 +647,6 @@ static bool search_nested_keyrings(struct key *keyring,
>> 	}
>> 
>> 	ctx->skipped_ret = 0;
>> -	if (ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK)
>> -		ctx->flags &= ~KEYRING_SEARCH_DO_STATE_CHECK;
>> 
>> 	/* Start processing a new keyring */
>> descend_to_keyring:
>> 
> 
> Reviewed-by: NeilBrown <neilb@suse.de>

Thanks!

> security/keys/internal.h says:
> 
> #define KEYRING_SEARCH_NO_STATE_CHECK   0x0001  /* Skip state checks */
> #define KEYRING_SEARCH_DO_STATE_CHECK   0x0002  /* Override NO_STATE_CHECK */
> 
> 
> Your match makes it obvious that DO overrides NO.  The current code
> doesn't get that right.
> 
> Is this on its way upstream yet (not in -next that I could see).

I’ve gotten no response on this patch. Maybe I sent it to the
wrong mailing list?

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Coddington Nov. 13, 2014, 3:29 p.m. UTC | #3 
On Thu, 13 Nov 2014, Chuck Lever wrote:

>
> On Nov 12, 2014, at 6:15 PM, NeilBrown <neilb@suse.de> wrote:
>
> > On Thu, 30 Oct 2014 13:46:47 -0400 Chuck Lever <chuck.lever@oracle.com> wrote:
> >
> >> After about 10 minutes, my NFSv4 functional tests fail because the
> >> ownership of the test files goes to "-2". Looking at /proc/keys
> >> shows that the id_resolv keys that map to my test user ID have
> >> expired. The ownership problem persists until the expired keys are
> >> purged from the keyring, and fresh keys are obtained.
> >>
> >> I bisected the problem to 3.13 commit b2a4df200d57 ("KEYS: Expand
> >> the capacity of a keyring"). This commit inadvertantly changes the
> >> API contract of the internal function keyring_search_aux().
> >>
> >> The root cause appears to be that b2a4df200d57 made "no state check"
> >> the default behavior. "No state check" means the keyring search
> >> iterator function skips checking the key's expiry timeout, and
> >> returns expired keys.  request_key_and_link() depends on getting
> >> an -EAGAIN result code to know when to perform an upcall to refresh
> >> an expired key.
> >>
> >> Restore the previous default behavior of keyring_search_aux() and
> >> the meaning of the KEYRING_SEARCH_{DO,NO}_STATE_CHECK flags.
> >>
> >> Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >> Resend with correct linux-nfs address.
> >>
> >> security/keys/keyring.c |   11 +++++++----
> >> 1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> >> index 8314a7d..1294582 100644
> >> --- a/security/keys/keyring.c
> >> +++ b/security/keys/keyring.c
> >> @@ -519,9 +519,14 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
> >> 	struct keyring_search_context *ctx = iterator_data;
> >> 	const struct key *key = keyring_ptr_to_key(object);
> >> 	unsigned long kflags = key->flags;
> >> +	bool state_check_needed;
> >>
> >> 	kenter("{%d}", key->serial);
> >>
> >> +	state_check_needed = !(ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK);
> >> +	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK)
> >> +		state_check_needed = true;
> >> +
> >> 	/* ignore keys not of this type */
> >> 	if (key->type != ctx->index_key.type) {
> >> 		kleave(" = 0 [!type]");
> >> @@ -529,7 +534,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
> >> 	}
> >>
> >> 	/* skip invalidated, revoked and expired keys */
> >> -	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
> >> +	if (state_check_needed) {
> >> 		if (kflags & ((1 << KEY_FLAG_INVALIDATED) |
> >> 			      (1 << KEY_FLAG_REVOKED))) {
> >> 			ctx->result = ERR_PTR(-EKEYREVOKED);
> >> @@ -559,7 +564,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
> >> 		goto skipped;
> >> 	}
> >>
> >> -	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
> >> +	if (state_check_needed) {
> >> 		/* we set a different error code if we pass a negative key */
> >> 		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
> >> 			smp_rmb();
> >> @@ -642,8 +647,6 @@ static bool search_nested_keyrings(struct key *keyring,
> >> 	}
> >>
> >> 	ctx->skipped_ret = 0;
> >> -	if (ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK)
> >> -		ctx->flags &= ~KEYRING_SEARCH_DO_STATE_CHECK;
> >>
> >> 	/* Start processing a new keyring */
> >> descend_to_keyring:
> >>
> >
> > Reviewed-by: NeilBrown <neilb@suse.de>
>
> Thanks!
>
> > security/keys/internal.h says:
> >
> > #define KEYRING_SEARCH_NO_STATE_CHECK   0x0001  /* Skip state checks */
> > #define KEYRING_SEARCH_DO_STATE_CHECK   0x0002  /* Override NO_STATE_CHECK */
> >
> >
> > Your match makes it obvious that DO overrides NO.  The current code
> > doesn't get that right.
> >
> > Is this on its way upstream yet (not in -next that I could see).
>
> I’ve gotten no response on this patch. Maybe I sent it to the
> wrong mailing list?

There's http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings for this,
but I think it's fairly broken.  My messages sent there have been
disappearing.  :/

Bruce, Trond - any idea what's wrong with the keyrings list?  I'd be
happy to attempt the repairs myself if time is the problem..

Ben
David Howells Nov. 14, 2014, 12:20 p.m. UTC | #4 
Chuck Lever <chuck.lever@oracle.com> wrote:

> -	if (ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK)
> -		ctx->flags &= ~KEYRING_SEARCH_DO_STATE_CHECK;

The problem is that this adversely affects keyring cycle checking and
possession checking.  Those absolutely must suppress the state check.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Howells Nov. 14, 2014, 2:49 p.m. UTC | #5 
I'm wondering if I actually need KEYRING_SEARCH_DO_STATE_CHECK and
KEYRING_SEARCH_NO_STATE_CHECK as separate flags rather than just states of the
same flag.

The most important distinction is in search_nested_keyrings() where I turn on
DO_STATE_CHECK specifically for potential matches on the root keyring.

However, NO_STATE_CHECK is only used for two special searches: possession
determination and cycle detection.  Neither of these use
keyring_search_iterator() as the iteration function, so neither actually takes
any notice of DO_STATE_CHECK.

Everything else currently uses - or should use - DO_STATE_CHECK, including
key_get_instantiation_authkey().

David
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chuck Lever III Nov. 14, 2014, 3:06 p.m. UTC | #6 
On Nov 14, 2014, at 6:20 AM, David Howells <dhowells@redhat.com> wrote:

> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> -	if (ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK)
>> -		ctx->flags &= ~KEYRING_SEARCH_DO_STATE_CHECK;
> 
> The problem is that this adversely affects keyring cycle checking and
> possession checking.  Those absolutely must suppress the state check.

Thanks for the review.

OK, I feared there could be side effects of restoring the original
API contract, but hoped there would not be.

It’s this code:

      if (ctx->match_data.lookup_type == KEYRING_SEARCH_LOOKUP_ITERATE ||
          keyring_compare_object(keyring, &ctx->index_key)) {
               ctx->skipped_ret = 2;
               ctx->flags |= KEYRING_SEARCH_DO_STATE_CHECK;

that flips DO_STATE_CHECK back on in some cases, right?

Any suggestions?

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chuck Lever III Nov. 14, 2014, 3:13 p.m. UTC | #7 
On Nov 14, 2014, at 8:49 AM, David Howells <dhowells@redhat.com> wrote:

> I'm wondering if I actually need KEYRING_SEARCH_DO_STATE_CHECK and
> KEYRING_SEARCH_NO_STATE_CHECK as separate flags rather than just states of the
> same flag.
> 
> The most important distinction is in search_nested_keyrings() where I turn on
> DO_STATE_CHECK specifically for potential matches on the root keyring.
> 
> However, NO_STATE_CHECK is only used for two special searches: possession
> determination and cycle detection.  Neither of these use
> keyring_search_iterator() as the iteration function, so neither actually takes
> any notice of DO_STATE_CHECK.
> 
> Everything else currently uses - or should use - DO_STATE_CHECK, including
> key_get_instantiation_authkey().

I replied earlier before reading all my mail. Thanks for posting
these, I’ll give them a try early next week.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Howells Nov. 14, 2014, 3:19 p.m. UTC | #8 
David Howells <dhowells@redhat.com> wrote:

> search_nested_keyrings() should probably honour KEYRING_SEARCH_NO_STATE_CHECK
> when checking the keyring at the root of its search by not setting
> DO_STATE_CHECK if NO_STATE_CHECK is set.
> 
> For the moment, this doesn't really matter as NO_STATE_CHECK is only used for
> cycle detection and possession determination, neither of which use
> keyring_search_iterator() as the search iterator check function, so
> DO_STATE_CHECK is actually ignored there.

Ignore this - I forgot to update the description.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 8314a7d..1294582 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -519,9 +519,14 @@  static int keyring_search_iterator(const void *object, void *iterator_data)
 	struct keyring_search_context *ctx = iterator_data;
 	const struct key *key = keyring_ptr_to_key(object);
 	unsigned long kflags = key->flags;
+	bool state_check_needed;
 
 	kenter("{%d}", key->serial);
 
+	state_check_needed = !(ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK);
+	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK)
+		state_check_needed = true;
+
 	/* ignore keys not of this type */
 	if (key->type != ctx->index_key.type) {
 		kleave(" = 0 [!type]");
@@ -529,7 +534,7 @@  static int keyring_search_iterator(const void *object, void *iterator_data)
 	}
 
 	/* skip invalidated, revoked and expired keys */
-	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
+	if (state_check_needed) {
 		if (kflags & ((1 << KEY_FLAG_INVALIDATED) |
 			      (1 << KEY_FLAG_REVOKED))) {
 			ctx->result = ERR_PTR(-EKEYREVOKED);
@@ -559,7 +564,7 @@  static int keyring_search_iterator(const void *object, void *iterator_data)
 		goto skipped;
 	}
 
-	if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
+	if (state_check_needed) {
 		/* we set a different error code if we pass a negative key */
 		if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
 			smp_rmb();
@@ -642,8 +647,6 @@  static bool search_nested_keyrings(struct key *keyring,
 	}
 
 	ctx->skipped_ret = 0;
-	if (ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK)
-		ctx->flags &= ~KEYRING_SEARCH_DO_STATE_CHECK;
 
 	/* Start processing a new keyring */
 descend_to_keyring: