| Message ID | 2659836.1607940186@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] keys: Collected minor fixes and cleanups | expand |
On Mon, Dec 14, 2020 at 2:04 AM David Howells <dhowells@redhat.com> wrote: > > Here's a set of minor fixes/cleanups that I've collected from various > people for the next merge window. This doesn't even build. And no, that's not because of some merge error on my part. Just to verify, I tried to build the head of what you sent me (commit 1b91ea77dfeb: "certs: Replace K{U,G}IDT_INIT() with GLOBAL_ROOT_{U,G}ID") and it fails the same way. In file included from ./include/linux/cred.h:13, from security/integrity/ima/ima_mok.c:12: security/integrity/ima/ima_mok.c: In function ‘ima_mok_init’: ./include/linux/key.h:292:29: warning: passing argument 7 of ‘keyring_alloc’ makes pointer from integer without a cast [-Wint-conversion] .. ten more lines of warnings.. security/integrity/ima/ima_mok.c:36:26: error: too many arguments to function ‘keyring_alloc’ 36 | ima_blacklist_keyring = keyring_alloc(".ima_blacklist", | ^~~~~~~~~~~~~ so these "fixes" have clearly had absolutely zero testing, haven't been in linux-next, and are completely broken. The bug was introduced by commit 33c36b2053de ("certs: Fix blacklist flag type confusion"), which changed the IMA code without actually testing it. I suspect the fix is trivial (change the "," to "|"), but I will not be pulling this - or anything else that hasn't been in linux-next - from you this merge window. The pain just isn't worth it, but more importantly, you simply need to get your workflow in order, and not send me completely untested garbage that hasn't even been compiled. Linus
On Mon, Dec 14, 2020 at 12:49 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > I suspect the fix is trivial (change the "," to "|"), but I will not > be pulling this - or anything else that hasn't been in linux-next - > from you this merge window. It looks like Stephen Rothwell saw it in next yesterday, and fixed it up there in his merge. So somebody was aware of the problem. But unlike Stephen, I don't take broken code and just silently fix it up in the merge. I suspect Stephen might have thought it was a merge conflict fix, rather than just a broken branch. Stephen: that makes linux-next test coverage kind of pointless, if you just fix bugs in the branches you merge. You should reject things more aggressively, rather than make them "pass" in Linux-next. Linus
Hi Linus, On Mon, 14 Dec 2020 13:05:51 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Mon, Dec 14, 2020 at 12:49 PM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > I suspect the fix is trivial (change the "," to "|"), but I will not > > be pulling this - or anything else that hasn't been in linux-next - > > from you this merge window. > > It looks like Stephen Rothwell saw it in next yesterday, and fixed it > up there in his merge. > > So somebody was aware of the problem. But unlike Stephen, I don't take > broken code and just silently fix it up in the merge. > > I suspect Stephen might have thought it was a merge conflict fix, > rather than just a broken branch. > > Stephen: that makes linux-next test coverage kind of pointless, if you > just fix bugs in the branches you merge. You should reject things more > aggressively, rather than make them "pass" in Linux-next. I also reported it last Friday (https://lore.kernel.org/lkml/20201211155031.0e35abf2@canb.auug.org.au/) and so assumed it would be fixed before being sent to you ... I sometimes fix simple things up but mostly reject them - clearly that would not have made a difference here.
On Mon, Dec 14, 2020 at 12:49:27PM -0800, Linus Torvalds wrote: > The pain just isn't worth it, but more importantly, you simply need to > get your workflow in order, and not send me completely untested > garbage that hasn't even been compiled. I have now more bandwidth. It was mostly eaten by SGX, especially last few months. Starting from next week, I'll start proactively test keyring changes (I'm this week on vacation). I've been thinking that maybe a two-folded approach would make sense for keyring: 1. I would pick fixes to my linux-tpmdd where they would get quickly mirrored to linux-next. It's already taking changes for trusted keys, i.e. not solely for TPM changes. 2. Feature changes would go through David's tree. > Linus /Jarkko
Hi Linus, Here's a set of minor fixes/cleanups that I've collected from various people for the next merge window. A couple of them might, in theory, be visible to userspace: (*) Make blacklist_vet_description() reject uppercase letters as they don't match the all-lowercase hex string generated for a blacklist search. This may want reconsideration in the future, but, currently, you can't add to the blacklist keyring from userspace and the only source of blacklist keys generates lowercase descriptions. (*) Fix blacklist_init() to use a new KEY_ALLOC_* flag to indicate that it wants KEY_FLAG_KEEP to be set rather than passing KEY_FLAG_KEEP into keyring_alloc() as KEY_FLAG_KEEP isn't a valid alloc flag. This isn't currently a problem as the blacklist keyring isn't currently writable by userspace. The rest of the patches are cleanups and I don't think they should have any visible effect. David --- The following changes since commit 85a2c56cb4454c73f56d3099d96942e7919b292f: Merge tag 'pm-5.10-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm (2020-11-26 11:17:37 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-fixes-20201214 for you to fetch changes up to 1b91ea77dfeb2c5924ab940f2e43177c78a37d8f: certs: Replace K{U,G}IDT_INIT() with GLOBAL_ROOT_{U,G}ID (2020-12-10 09:24:43 +0000) ---------------------------------------------------------------- Keys fixes ---------------------------------------------------------------- Alex Shi (2): PKCS#7: drop function from kernel-doc pkcs7_validate_trust_one certs/blacklist: fix kernel doc interface issue Alexander A. Klimov (1): encrypted-keys: Replace HTTP links with HTTPS ones David Howells (1): certs: Fix blacklist flag type confusion Denis Efremov (1): security/keys: use kvfree_sensitive() Gabriel Krisman Bertazi (1): watch_queue: Drop references to /dev/watch_queue Gustavo A. R. Silva (1): security: keys: Fix fall-through warnings for Clang Jann Horn (1): keys: Remove outdated __user annotations Krzysztof Kozlowski (1): KEYS: asymmetric: Fix kerneldoc Mickaël Salaün (3): certs: Fix blacklisted hexadecimal hash string check PKCS#7: Fix missing include certs: Replace K{U,G}IDT_INIT() with GLOBAL_ROOT_{U,G}ID Randy Dunlap (2): security: keys: delete repeated words in comments crypto: asymmetric_keys: fix some comments in pkcs7_parser.h Tianjia Zhang (1): crypto: public_key: Remove redundant header file from public_key.h Tom Rix (2): KEYS: remove redundant memset keys: remove trailing semicolon in macro definition YueHaibing (1): crypto: pkcs7: Use match_string() helper to simplify the code Documentation/security/keys/core.rst | 4 ++-- certs/blacklist.c | 10 +++++----- certs/system_keyring.c | 5 +++-- crypto/asymmetric_keys/asymmetric_type.c | 6 ++++-- crypto/asymmetric_keys/pkcs7_parser.h | 5 ++--- crypto/asymmetric_keys/pkcs7_trust.c | 2 +- crypto/asymmetric_keys/pkcs7_verify.c | 9 ++++----- include/crypto/public_key.h | 1 - include/keys/encrypted-type.h | 2 +- include/linux/key.h | 5 +++-- include/linux/verification.h | 2 ++ samples/Kconfig | 2 +- samples/watch_queue/watch_test.c | 2 +- security/integrity/ima/ima_mok.c | 3 +-- security/keys/Kconfig | 8 ++++---- security/keys/big_key.c | 9 +++------ security/keys/key.c | 2 ++ security/keys/keyctl.c | 2 +- security/keys/keyctl_pkey.c | 2 -- security/keys/keyring.c | 10 +++++----- security/keys/process_keys.c | 1 + 21 files changed, 46 insertions(+), 46 deletions(-)