| Message ID | 20141114153923.21180.66516.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 key > until that key gets gc’d. That’s 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 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. As long as request_key_and_link() can’t return an expired key. Another option is to alter the NFS idmapper logic to match the current keyring API contract. I didn’t choose that option before because it seemed like it wasn’t 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 -- 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 <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 -- 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 --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;
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(-) -- 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