• Eric Biggers's avatar
    fscrypt: stop using keyrings subsystem for fscrypt_master_key · d7e7b9af
    Eric Biggers authored
    The approach of fs/crypto/ internally managing the fscrypt_master_key
    structs as the payloads of "struct key" objects contained in a
    "struct key" keyring has outlived its usefulness.  The original idea was
    to simplify the code by reusing code from the keyrings subsystem.
    However, several issues have arisen that can't easily be resolved:
    
    - When a master key struct is destroyed, blk_crypto_evict_key() must be
      called on any per-mode keys embedded in it.  (This started being the
      case when inline encryption support was added.)  Yet, the keyrings
      subsystem can arbitrarily delay the destruction of keys, even past the
      time the filesystem was unmounted.  Therefore, currently there is no
      easy way to call blk_crypto_evict_key() when a master key is
      destroyed.  Currently, this is worked around by holding an extra
      reference to the filesystem's request_queue(s).  But it was overlooked
      that the request_queue reference is *not* guaranteed to pin the
      corresponding blk_crypto_profile too; for device-mapper devices that
      support inline crypto, it doesn't.  This can cause a use-after-free.
    
    - When the last inode that was using an incompletely-removed master key
      is evicted, the master key removal is completed by removing the key
      struct from the keyring.  Currently this is done via key_invalidate().
      Yet, key_invalidate() takes the key semaphore.  This can deadlock when
      called from the shrinker, since in fscrypt_ioctl_add_key(), memory is
      allocated with GFP_KERNEL under the same semaphore.
    
    - More generally, the fact that the keyrings subsystem can arbitrarily
      delay the destruction of keys (via garbage collection delay, or via
      random processes getting temporary key references) is undesirable, as
      it means we can't strictly guarantee that all secrets are ever wiped.
    
    - Doing the master key lookups via the keyrings subsystem results in the
      key_permission LSM hook being called.  fscrypt doesn't want this, as
      all access control for encrypted files is designed to happen via the
      files themselves, like any other files.  The workaround which SELinux
      users are using is to change their SELinux policy to grant key search
      access to all domains.  This works, but it is an odd extra step that
      shouldn't really have to be done.
    
    The fix for all these issues is to change the implementation to what I
    should have done originally: don't use the keyrings subsystem to keep
    track of the filesystem's fscrypt_master_key structs.  Instead, just
    store them in a regular kernel data structure, and rework the reference
    counting, locking, and lifetime accordingly.  Retain support for
    RCU-mode key lookups by using a hash table.  Replace fscrypt_sb_free()
    with fscrypt_sb_delete(), which releases the keys synchronously and runs
    a bit earlier during unmount, so that block devices are still available.
    
    A side effect of this patch is that neither the master keys themselves
    nor the filesystem keyrings will be listed in /proc/keys anymore.
    ("Master key users" and the master key users keyrings will still be
    listed.)  However, this was mostly an implementation detail, and it was
    intended just for debugging purposes.  I don't know of anyone using it.
    
    This patch does *not* change how "master key users" (->mk_users) works;
    that still uses the keyrings subsystem.  That is still needed for key
    quotas, and changing that isn't necessary to solve the issues listed
    above.  If we decide to change that too, it would be a separate patch.
    
    I've marked this as fixing the original commit that added the fscrypt
    keyring, but as noted above the most important issue that this patch
    fixes wasn't introduced until the addition of inline encryption support.
    
    Fixes: 22d94f49 ("fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl")
    Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
    Link: https://lore.kernel.org/r/20220901193208.138056-2-ebiggers@kernel.org
    d7e7b9af
policy.c 25.9 KB