[PATCH] KEYS: Ensure expired keys are renewed

[PATCH] KEYS: Ensure expired keys are renewed

[Chuck Lever]

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:

Re: [PATCH] KEYS: Ensure expired keys are renewed

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3627 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] KEYS: Ensure expired keys are renewed

[Chuck Lever]

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

Re: [PATCH] KEYS: Ensure expired keys are renewed

[Benjamin Coddington]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4252 bytes --]


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

Re: [PATCH] KEYS: Ensure expired keys are renewed

[David Howells]

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

Re: [PATCH] KEYS: Ensure expired keys are renewed

[Chuck Lever]

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

[Keyrings] [PATCH 1/3] KEYS: request_key_and_link() needs to request state checks when searching

[David Howells]

request_key_and_link() needs to request state checks be performed on the
potentially extant keys when it requests a search of the process keyrings.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/request_key.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index bb4337c7ae1b..0bb23f98e4ca 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -516,6 +516,7 @@ struct key *request_key_and_link(struct key_type *type,
 		.match_data.cmp		= key_default_cmp,
 		.match_data.raw_data	= description,
 		.match_data.lookup_type	= KEYRING_SEARCH_LOOKUP_DIRECT,
+		.flags			= KEYRING_SEARCH_DO_STATE_CHECK,
 	};
 	struct key *key;
 	key_ref_t key_ref;

_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings

[Keyrings] [PATCH 2/3] KEYS: When searching a keyring, restore KEYRING_SEARCH_DO_STATE_CHECK

[David Howells]

When searching a keyring or iterating over all the contents of a keyring, we
set KEYRING_SEARCH_DO_STATE_CHECK before checking the root keyring so that the
iterator function will ensure that we have permission to search that keyring.

However, we should restore the value of the flag afterwards as it will
otherwise affect all other keys checked by the iterator.

Signed-off-by: David Howells <dhowells@redhat.com>
---

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

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 8177010174f7..f44b3a8d605a 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -636,6 +636,7 @@ static bool search_nested_keyrings(struct key *keyring,
 	 */
 	if (ctx->match_data.lookup_type == KEYRING_SEARCH_LOOKUP_ITERATE ||
 	    keyring_compare_object(keyring, &ctx->index_key)) {
+		unsigned long saved_flags = ctx->flags;
 		ctx->skipped_ret = 2;
 		ctx->flags |= KEYRING_SEARCH_DO_STATE_CHECK;
 		switch (ctx->iterator(keyring_key_to_ptr(keyring), ctx)) {
@@ -644,6 +645,7 @@ static bool search_nested_keyrings(struct key *keyring,
 		case 2:
 			return false;
 		default:
+			ctx->flags = saved_flags;
 			break;
 		}
 	}

_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings

[Keyrings] [PATCH 3/3] KEYS: KEYRING_SEARCH_NO_STATE_CHECK overrides KEYRING_SEARCH_DO_STATE_CHECK

[David Howells]

KEYRING_SEARCH_NO_STATE_CHECK overrides KEYRING_SEARCH_DO_STATE_CHECK because
it's needed for special operations like keyring cycle detection and possession
detection.  Fix the header comment to this effect.

Signed-off-by: David Howells <dhowells@redhat.com>
---

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

diff --git a/security/keys/internal.h b/security/keys/internal.h
index b8960c4959a5..9e77e33bf8bc 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -112,8 +112,8 @@ struct keyring_search_context {
 	const struct cred	*cred;
 	struct key_match_data	match_data;
 	unsigned		flags;
-#define KEYRING_SEARCH_NO_STATE_CHECK	0x0001	/* Skip state checks */
-#define KEYRING_SEARCH_DO_STATE_CHECK	0x0002	/* Override NO_STATE_CHECK */
+#define KEYRING_SEARCH_NO_STATE_CHECK	0x0001	/* Skip state checks (overrides DO_STATE_CHECK) */
+#define KEYRING_SEARCH_DO_STATE_CHECK	0x0002	/* Request state checks */
 #define KEYRING_SEARCH_NO_UPDATE_TIME	0x0004	/* Don't update times */
 #define KEYRING_SEARCH_NO_CHECK_PERM	0x0008	/* Don't check permissions */
 #define KEYRING_SEARCH_DETECT_TOO_DEEP	0x0010	/* Give an error on excessive depth */

_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings

Are both DO_STATE_CHECK and NO_STATE_CHECK required?

[David Howells]

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

Re: [Keyrings] Are both DO_STATE_CHECK and NO_STATE_CHECK required?

[Chuck Lever]

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 o=
f the
> same flag.
> =

> The most important distinction is in search_nested_keyrings() where I tur=
n 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=92ll give them a try early next week.

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



_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/c=
gi-bin/mailman/listinfo/keyrings

[Keyrings] [PATCH] KEYS: search_nested_keyrings() should honour NO_STATE_CHECK for the root

[David Howells]

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.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/keyring.c          |    7 ++++---
 security/keys/request_key.c      |    1 +
 security/keys/request_key_auth.c |    1 +
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 8177010174f7..41166f38a6e9 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -628,6 +628,10 @@ static bool search_nested_keyrings(struct key *keyring,
 	       ctx->index_key.type->name,
 	       ctx->index_key.description);
 
+#define STATE_CHECKS (KEYRING_SEARCH_NO_STATE_CHECK | KEYRING_SEARCH_DO_STATE_CHECK)
+	BUG_ON((ctx->flags & STATE_CHECKS) == 0 ||
+	       (ctx->flags & STATE_CHECKS) == STATE_CHECKS);
+	
 	if (ctx->index_key.description)
 		ctx->index_key.desc_len = strlen(ctx->index_key.description);
 
@@ -637,7 +641,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	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;
 		switch (ctx->iterator(keyring_key_to_ptr(keyring), ctx)) {
 		case 1:
 			goto found;
@@ -649,8 +652,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:
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index bb4337c7ae1b..0bb23f98e4ca 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -516,6 +516,7 @@ struct key *request_key_and_link(struct key_type *type,
 		.match_data.cmp		= key_default_cmp,
 		.match_data.raw_data	= description,
 		.match_data.lookup_type	= KEYRING_SEARCH_LOOKUP_DIRECT,
+		.flags			= KEYRING_SEARCH_DO_STATE_CHECK,
 	};
 	struct key *key;
 	key_ref_t key_ref;
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 6639e2cb8853..5d672f7580dd 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -249,6 +249,7 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
 		.match_data.cmp		= key_default_cmp,
 		.match_data.raw_data	= description,
 		.match_data.lookup_type	= KEYRING_SEARCH_LOOKUP_DIRECT,
+		.flags			= KEYRING_SEARCH_DO_STATE_CHECK,
 	};
 	struct key *authkey;
 	key_ref_t authkey_ref;

_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings

Re: [Keyrings] [PATCH] KEYS: search_nested_keyrings() should honour NO_STATE_CHECK for the root

[David Howells]

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.
_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings

[Keyrings] [PATCH] KEYS: Simplify KEYRING_SEARCH_{NO, DO}_STATE_CHECK flags

[David Howells]

Simplify KEYRING_SEARCH_{NO,DO}_STATE_CHECK flags to be two variations of the
same flag.  They are effectively mutually exclusive and one or the other
should be provided, but not both.

Keyring cycle detection and key possession determination are the only things
that set NO_STATE_CHECK, except that neither flag really does anything there
because neither purpose makes use of the keyring_search_iterator() function,
but rather provides their own.

For cycle detection we definitely want to check inside of expired keyrings,
just so that we don't create a cycle we can't get rid of.  Revoked keyrings
are cleared at revocation time and can't then be reused, so shouldn't be a
problem either way.

For possession determination, we *might* want to validate each keyring before
searching it: do you possess a key that's hidden behind an expired or just
plain inaccessible keyring?  Currently, the answer is yes.  Note that you
cannot, however, possess a key behind a revoked keyring because they are
cleared on revocation.

keyring_search() sets DO_STATE_CHECK, which is correct.

request_key_and_link() currently doesn't specify whether to check the key
state or not - but it should set DO_STATE_CHECK.

key_get_instantiation_authkey() also currently doesn't specify whether to
check the key state or not - but it probably should also set DO_STATE_CHECK.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/keyring.c          |    7 ++++---
 security/keys/request_key.c      |    1 +
 security/keys/request_key_auth.c |    1 +
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 8177010174f7..238aa172f25b 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -628,6 +628,10 @@ static bool search_nested_keyrings(struct key *keyring,
 	       ctx->index_key.type->name,
 	       ctx->index_key.description);
 
+#define STATE_CHECKS (KEYRING_SEARCH_NO_STATE_CHECK | KEYRING_SEARCH_DO_STATE_CHECK)
+	BUG_ON((ctx->flags & STATE_CHECKS) == 0 ||
+	       (ctx->flags & STATE_CHECKS) == STATE_CHECKS);
+
 	if (ctx->index_key.description)
 		ctx->index_key.desc_len = strlen(ctx->index_key.description);
 
@@ -637,7 +641,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	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;
 		switch (ctx->iterator(keyring_key_to_ptr(keyring), ctx)) {
 		case 1:
 			goto found;
@@ -649,8 +652,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:
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index bb4337c7ae1b..0bb23f98e4ca 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -516,6 +516,7 @@ struct key *request_key_and_link(struct key_type *type,
 		.match_data.cmp		= key_default_cmp,
 		.match_data.raw_data	= description,
 		.match_data.lookup_type	= KEYRING_SEARCH_LOOKUP_DIRECT,
+		.flags			= KEYRING_SEARCH_DO_STATE_CHECK,
 	};
 	struct key *key;
 	key_ref_t key_ref;
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index 6639e2cb8853..5d672f7580dd 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -249,6 +249,7 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
 		.match_data.cmp		= key_default_cmp,
 		.match_data.raw_data	= description,
 		.match_data.lookup_type	= KEYRING_SEARCH_LOOKUP_DIRECT,
+		.flags			= KEYRING_SEARCH_DO_STATE_CHECK,
 	};
 	struct key *authkey;
 	key_ref_t authkey_ref;

_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings

Re: [Keyrings] [PATCH] KEYS: Simplify KEYRING_SEARCH_{NO, DO}_STATE_CHECK flags

[David Howells]

I'm not sure this patch actually solves your problem.

> request_key_and_link() depends on getting an -EAGAIN result code to know
> when to perform an upcall to refresh an expired key.

request_key_and_link() should return EKEYEXPIRED if it meets an expired key
until that key gets gc'd.

What we lack is that bit to upcall to refresh the expired key.
/sbin/request-key can support it - the first column has 'create' for key
creation and can hold other values for updating a key and KEYCTL_UPDATE can be
allowed to unexpire a key.

Possibly I should be only returning EKEYEXPIRED if the key instantiation was
rejected so and simply invalidate the key if it's in-memory expiration
occurs.  Making this so will cause failures in the testsuite, but I think
that's okay.

Another option is to allow keys to be specifically marked at
immediate-gc-on-expire such that you never see them in the expired state
unless you're holding a ref on one inside the kernel.

David
_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings

Re: [Keyrings] [PATCH] KEYS: Simplify KEYRING_SEARCH_{NO, DO}_STATE_CHECK flags

[Chuck Lever]

Hi David-

On Nov 17, 2014, at 10:08 AM, David Howells <dhowells@redhat.com> wrote:

> I'm not sure this patch actually solves your problem.
> =

>> request_key_and_link() depends on getting an -EAGAIN result code to know
>> when to perform an upcall to refresh an expired key.
> =

> request_key_and_link() should return EKEYEXPIRED if it meets an expired k=
ey
> until that key gets gc=92d.

That=92s what the documenting comment says, but...

nfs_idmap_request_key() considers any error return from
request_key() to be a reason to fall back to using the
legacy (non-keyring-based) ID mapper.

nfs_idmap_get_key() considers any error return from
key_validate() to be a permanent error.

IOW the NFS idmapper logic depends on getting a valid key
back from request_key() ie, one that has not expired,
or one that was just refreshed. Getting back an expired
key or getting -EKEYEXPIRED is unexpected.

Thus it appears to be an API contract change. At least,
the documenting comment in front of request_key_and_link()
does not seem to match the current design of the NFS
idmapper code.

> What we lack is that bit to upcall to refresh the expired key.

Currently request_key_and_link() expects -EAGAIN from
search_process_keyrings() to know when it should upcall.
But, maybe something else is needed there instead.

> /sbin/request-key can support it - the first column has 'create' for key
> creation and can hold other values for updating a key and KEYCTL_UPDATE c=
an be
> allowed to unexpire a key.
> =

> Possibly I should be only returning EKEYEXPIRED if the key instantiation =
was
> rejected so and simply invalidate the key if it's in-memory expiration
> occurs.  Making this so will cause failures in the testsuite, but I think
> that's okay.
> =

> Another option is to allow keys to be specifically marked at
> immediate-gc-on-expire such that you never see them in the expired state
> unless you're holding a ref on one inside the kernel.

As long as request_key_and_link() can=92t return an
expired key.

Another option is to alter the NFS idmapper logic
to match the current keyring API contract. I didn=92t
choose that option before because it seemed like it
wasn=92t the intention to change that contract in
3.13.

I'll hold off on testing the patches you sent over
the weekend.

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



_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/c=
gi-bin/mailman/listinfo/keyrings

Re: [Keyrings] [PATCH] KEYS: Simplify KEYRING_SEARCH_{NO, DO}_STATE_CHECK flags

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> I'm not sure this patch actually solves your problem.
> 
> > request_key_and_link() depends on getting an -EAGAIN result code to know
> > when to perform an upcall to refresh an expired key.
> 
> request_key_and_link() should return EKEYEXPIRED if it meets an expired key
> until that key gets gc'd.
> 
> What we lack is that bit to upcall to refresh the expired key.
> /sbin/request-key can support it - the first column has 'create' for key
> creation and can hold other values for updating a key and KEYCTL_UPDATE can be
> allowed to unexpire a key.
> 
> Possibly I should be only returning EKEYEXPIRED if the key instantiation was
> rejected so and simply invalidate the key if it's in-memory expiration
> occurs.  Making this so will cause failures in the testsuite, but I think
> that's okay.
> 
> Another option is to allow keys to be specifically marked at
> immediate-gc-on-expire such that you never see them in the expired state
> unless you're holding a ref on one inside the kernel.

Having thought about it some more, I think the thing to do is to have
request_key() do as add_key() does and either replace or update a key that's
locally expired.

A key that was rejected (negatively instantiated) with EKEYEXPIRED as the
error to present should present that error instead with request_key() is
invoked.

Invalidated keys shouldn't be returned by the search algorithm.

Revoked keys should probably fail with EKEYREVOKED.  Invalidation rather than
revocation should be used to evict keys from memory.

Keyctl functions that access an expired key for other purposes, such as to
read the contents, should fail with EKEYEXPIRED still (apart from unlink and
invalidate which should always succeed given appropriate permissions).

I'm going to work on a patch on this basis.

David
_______________________________________________
Keyrings mailing list
Keyrings@linux-nfs.org
To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings