linux-ext4 - Re: [PATCH v11 02/12] block: Keyslot Manager for Inline Encryption

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200430064621.GA16238@sol.localdomain>
Date:   Wed, 29 Apr 2020 23:46:21 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Satya Tangirala <satyat@...gle.com>
Cc:     linux-block@...r.kernel.org, linux-scsi@...r.kernel.org,
        linux-fscrypt@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net, linux-ext4@...r.kernel.org,
        Barani Muthukumaran <bmuthuku@....qualcomm.com>,
        Kuohong Wang <kuohong.wang@...iatek.com>,
        Kim Boojin <boojin.kim@...sung.com>
Subject: Re: [PATCH v11 02/12] block: Keyslot Manager for Inline Encryption

A few very minor comments:

On Wed, Apr 29, 2020 at 07:21:11AM +0000, Satya Tangirala wrote:
> diff --git a/block/keyslot-manager.c b/block/keyslot-manager.c
> new file mode 100644
> index 0000000000000..b584723b392ad
> --- /dev/null
> +++ b/block/keyslot-manager.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 Google LLC
> + */
> +
> +/**
> + * DOC: The Keyslot Manager
> + *
> + * Many devices with inline encryption support have a limited number of "slots"
> + * into which encryption contexts may be programmed, and requests can be tagged
> + * with a slot number to specify the key to use for en/decryption.
> + *
> + * As the number of slots are limited, and programming keys is expensive on
> + * many inline encryption hardware, we don't want to program the same key into
> + * multiple slots - if multiple requests are using the same key, we want to
> + * program just one slot with that key and use that slot for all requests.
> + *
> + * The keyslot manager manages these keyslots appropriately, and also acts as
> + * an abstraction between the inline encryption hardware and the upper layers.
> + *
> + * Lower layer devices will set up a keyslot manager in their request queue
> + * and tell it how to perform device specific operations like programming/
> + * evicting keys from keyslots.
> + *
> + * Upper layers will call blk_ksm_get_slot_for_key() to program a
> + * key into some slot in the inline encryption hardware.
> + */
> +#include <crypto/algapi.h>

Now that this file doesn't use crypto_memneq(), the include of <crypto/algapi.h>
can be removed.

> +/**
> + * blk_ksm_get_slot_for_key() - Program a key into a keyslot.
> + * @ksm: The keyslot manager to program the key into.
> + * @key: Pointer to the key object to program, including the raw key, crypto
> + *	 mode, and data unit size.
> + * @keyslot: A pointer to return the pointer of the allocated keyslot.
> + *
> + * Get a keyslot that's been programmed with the specified key.  If one already
> + * exists, return it with incremented refcount.  Otherwise, wait for a keyslot
> + * to become idle and program it.
> + *
> + * Context: Process context. Takes and releases ksm->lock.
> + * Return: BLK_STS_OK on success (and keyslot is set to the pointer of the
> + *	   allocated keyslot), or some other blk_status_t otherwise (and
> + *	   keyslot is set to NULL).
> + */
> +blk_status_t blk_ksm_get_slot_for_key(struct blk_keyslot_manager *ksm,
> +				      const struct blk_crypto_key *key,
> +				      struct blk_ksm_keyslot **slot_ptr)

The comment should say @slot_ptr, not @keyslot.  You can find kerneldoc warnings
using 'scripts/kernel-doc -v -none $file'.

> +/**
> + * blk_ksm_crypto_cfg_supported() - Find out if the crypto_mode, dusize, dun
> + *				    bytes of a crypto_config are supported by a
> + *				    ksm.

IMO, shorten this a bit to something like "Find out if a crypto configuration is
supported by a ksm", so that less of the comment becomes outdated when someone
adds a new field.

> + * @ksm: The keyslot manager to check
> + * @cfg: The crypto configuration to check for.
> + *
> + * Checks for crypto_mode/data unit size/dun bytes support.
> + *
> + * Return: Whether or not this ksm supports the specified crypto key.

"config", not "key".

- Eric