Control: tags -1 + patch
Hi
On Thu, Oct 05, 2017 at 09:06:33AM +0200, Salvatore Bonaccorso wrote:
> Source: linux
> Version: 3.16.7-ckt7-1
> Severity: normal
>
> Hi
>
> In 3.16.7-ckt7-1 we applied a backport of "EYS: request_key() should
> reget expired keys rather than give EKEYEXPIRED", adressing #758870,
> 0b0a84154eff56913e91df29de5c3a03a0029e38.
[...]
> Are we potentially miss any relevant needed commits between v3.16..v3.18?
> (054f6180d8b5602b431b5924976c956e760488b1, "KEYS: Simplify
> KEYRING_SEARCH_{NO,DO}_STATE_CHECK flags"?).
I did some bisecting, and indeed it seems we were missing the second
change as mentioned as well in the merge:
https://git.kernel.org/linus/23c836ce5c1e1e0bb942f58a3cbc2f7fc05a08b5
> The second and third fix a bug in NFS idmapper handling whereby a key
> representing a mapping between an id and a name expires and causing
> EKEYEXPIRED to be seen internally in NFS (which prevents the mapping
> from happening) rather than re-looking up the mapping"
With attached patch on top of the (current) jessie branch in git, the
problem is solved.
Regards,
Salvatore
>From 83a57b39cea1cdd82e42483dc5a60f671c32252c Mon Sep 17 00:00:00 2001
From: Salvatore Bonaccorso <car...@debian.org>
Date: Thu, 5 Oct 2017 17:16:34 +0200
Subject: [PATCH] KEYS: Simplify KEYRING_SEARCH_{NO,DO}_STATE_CHECK flags
Closes: #877760
---
debian/changelog | 3 +
...fy-KEYRING_SEARCH_-NO-DO-_STATE_CHECK-fla.patch | 96 ++++++++++++++++++++++
...t_key-should-reget-expired-keys-rather-th.patch | 8 +-
debian/patches/series | 1 +
4 files changed, 104 insertions(+), 4 deletions(-)
create mode 100644 debian/patches/bugfix/all/KEYS-Simplify-KEYRING_SEARCH_-NO-DO-_STATE_CHECK-fla.patch
diff --git a/debian/changelog b/debian/changelog
index 55ac046e7..9bb4229ac 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -582,6 +582,9 @@ linux (3.16.48-1) UNRELEASED; urgency=medium
- ext4: preserve i_mode if __ext4_set_acl() fails
- ext4: Don't clear SGID when inheriting ACLs
+ [ Salvatore Bonaccorso ]
+ * KEYS: Simplify KEYRING_SEARCH_{NO,DO}_STATE_CHECK flags (Closes: #877760)
+
-- Ben Hutchings <b...@decadent.org.uk> Sun, 06 Aug 2017 22:03:56 +0100
linux (3.16.43-2+deb8u5) jessie-security; urgency=medium
diff --git a/debian/patches/bugfix/all/KEYS-Simplify-KEYRING_SEARCH_-NO-DO-_STATE_CHECK-fla.patch b/debian/patches/bugfix/all/KEYS-Simplify-KEYRING_SEARCH_-NO-DO-_STATE_CHECK-fla.patch
new file mode 100644
index 000000000..44f9caee4
--- /dev/null
+++ b/debian/patches/bugfix/all/KEYS-Simplify-KEYRING_SEARCH_-NO-DO-_STATE_CHECK-fla.patch
@@ -0,0 +1,96 @@
+From: David Howells <dhowe...@redhat.com>
+Date: Mon, 1 Dec 2014 22:52:50 +0000
+Subject: KEYS: Simplify KEYRING_SEARCH_{NO,DO}_STATE_CHECK flags
+Origin: https://git.kernel.org/linus/054f6180d8b5602b431b5924976c956e760488b1
+
+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 <dhowe...@redhat.com>
+Tested-by: Chuck Lever <chuck.le...@oracle.com>
+[carnil: Backported to 3.16: Adjust context]
+---
+ 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(-)
+
+--- a/security/keys/keyring.c
++++ b/security/keys/keyring.c
+@@ -609,6 +609,10 @@ static bool search_nested_keyrings(struc
+ 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);
+
+@@ -618,7 +622,6 @@ static bool search_nested_keyrings(struc
+ if (ctx->flags & 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;
+@@ -630,8 +633,6 @@ static bool search_nested_keyrings(struc
+ }
+
+ 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:
+--- a/security/keys/request_key.c
++++ b/security/keys/request_key.c
+@@ -536,7 +536,8 @@ struct key *request_key_and_link(struct
+ .cred = current_cred(),
+ .match = type->match,
+ .match_data = description,
+- .flags = KEYRING_SEARCH_LOOKUP_DIRECT,
++ .flags = (KEYRING_SEARCH_LOOKUP_DIRECT |
++ KEYRING_SEARCH_DO_STATE_CHECK),
+ };
+ struct key *key;
+ key_ref_t key_ref;
+--- a/security/keys/request_key_auth.c
++++ b/security/keys/request_key_auth.c
+@@ -235,7 +235,8 @@ struct key *key_get_instantiation_authke
+ .cred = current_cred(),
+ .match = user_match,
+ .match_data = description,
+- .flags = KEYRING_SEARCH_LOOKUP_DIRECT,
++ .flags = (KEYRING_SEARCH_LOOKUP_DIRECT |
++ KEYRING_SEARCH_DO_STATE_CHECK),
+ };
+ struct key *authkey;
+ key_ref_t authkey_ref;
diff --git a/debian/patches/bugfix/all/keys-request_key-should-reget-expired-keys-rather-th.patch b/debian/patches/bugfix/all/keys-request_key-should-reget-expired-keys-rather-th.patch
index 842c5803f..daf861c39 100644
--- a/debian/patches/bugfix/all/keys-request_key-should-reget-expired-keys-rather-th.patch
+++ b/debian/patches/bugfix/all/keys-request_key-should-reget-expired-keys-rather-th.patch
@@ -90,12 +90,12 @@ Tested-by: Chuck Lever <chuck.le...@oracle.com>
}
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
-@@ -533,7 +533,8 @@ struct key *request_key_and_link(struct
- .cred = current_cred(),
+@@ -537,7 +537,8 @@ struct key *request_key_and_link(struct
.match = type->match,
.match_data = description,
-- .flags = KEYRING_SEARCH_LOOKUP_DIRECT,
-+ .flags = (KEYRING_SEARCH_LOOKUP_DIRECT |
+ .flags = (KEYRING_SEARCH_LOOKUP_DIRECT |
+- KEYRING_SEARCH_DO_STATE_CHECK),
++ KEYRING_SEARCH_DO_STATE_CHECK |
+ KEYRING_SEARCH_SKIP_EXPIRED),
};
struct key *key;
diff --git a/debian/patches/series b/debian/patches/series
index eab75e0bb..0736ee441 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -222,6 +222,7 @@ bugfix/all/aufs-move-d_rcu-from-overlapping-d_child-to-overlapping-d.patch
bugfix/all/net-mv643xx-disable-tso-by-default.patch
bugfix/all/xen-balloon-cancel-ballooning-if-adding-new-memory-f.patch
bugfix/all/xen-balloon-Don-t-continue-ballooning-when-BP_ECANCE.patch
+bugfix/all/KEYS-Simplify-KEYRING_SEARCH_-NO-DO-_STATE_CHECK-fla.patch
bugfix/all/keys-request_key-should-reget-expired-keys-rather-th.patch
bugfix/all/hid-thingm-fix-workqueue-race-on-remove.patch
debian/emmc-don-t-initialize-partitions-on-rpmb-flagged-areas.patch
--
2.14.2
