mbox series

[GIT,PULL] keys: Collected minor fixes and cleanups

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

Pull-request

git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-fixes-20201214

Message

David Howells Dec. 14, 2020, 10:03 a.m. UTC
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(-)

Comments

Linus Torvalds Dec. 14, 2020, 8:49 p.m. UTC | #1
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
Linus Torvalds Dec. 14, 2020, 9:05 p.m. UTC | #2
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
Stephen Rothwell Dec. 14, 2020, 9:40 p.m. UTC | #3
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.
Jarkko Sakkinen Dec. 15, 2020, 4:57 a.m. UTC | #4
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