Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 2 Aug 2022 15:48:34 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Evan Green <evgreen@chromium.org>
Cc:     linux-kernel@vger.kernel.org,
        Matthew Garrett <mgarrett@aurora.tech>, dlunev@google.com,
        zohar@linux.ibm.com, jejb@linux.ibm.com,
        linux-integrity@vger.kernel.org, corbet@lwn.net, rjw@rjwysocki.net,
        gwendal@chromium.org, jarkko@kernel.org, linux-pm@vger.kernel.org,
        Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        "Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH 08/10] PM: hibernate: Mix user key in encrypted hibernate
Message-ID: <YumpwkIz+S+zDfol@sol.localdomain>
References: <20220504232102.469959-1-evgreen@chromium.org>
 <20220504161439.8.I87952411cf83f2199ff7a4cc8c828d357b8c8ce3@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220504161439.8.I87952411cf83f2199ff7a4cc8c828d357b8c8ce3@changeid>
Precedence: bulk

On Wed, May 04, 2022 at 04:21:00PM -0700, Evan Green wrote:
> +/*
> + * Allow user mode to fold in key material for the data portion of the hibernate
> + * image.
> + */
> +struct uswsusp_user_key {
> +	/* Kernel returns the metadata size. */
> +	__kernel_loff_t meta_size;
> +	__u32 key_len;
> +	__u8 key[16];
> +	__u32 pad;
> +};

Shouldn't the key field be 32 bytes?

> +/* Derive a key from the kernel and user keys for data encryption. */
> +static int snapshot_use_user_key(struct snapshot_data *data)
> +{
> +	struct shash_desc *desc;
> +	u8 digest[SHA256_DIGEST_SIZE];
> +	struct trusted_key_payload *payload;
> +	struct crypto_shash *tfm;
> +	int ret;
> +
> +	tfm = crypto_alloc_shash("sha256", 0, 0);
> +	if (IS_ERR(tfm)) {
> +		ret = -EINVAL;
> +		goto err_rel;
> +	}
> +
> +	desc = kmalloc(sizeof(struct shash_desc) +
> +		       crypto_shash_descsize(tfm), GFP_KERNEL);
> +	if (!desc) {
> +		ret = -ENOMEM;
> +		goto err_rel;
> +	}
> +
> +	desc->tfm = tfm;
> +	ret = crypto_shash_init(desc);
> +	if (ret != 0)
> +		goto err_free;
> +
> +	/*
> +	 * Hash the kernel key and the user key together. This folds in the user
> +	 * key, but not in a way that gives the user mode predictable control
> +	 * over the key bits. Hash in all 32 bytes of the key even though only 16
> +	 * are in active use as extra salt.
> +	 */
> +	payload = data->key->payload.data[0];
> +	crypto_shash_update(desc, payload->key, MIN_KEY_SIZE);
> +	crypto_shash_update(desc, data->user_key, sizeof(data->user_key));
> +	crypto_shash_final(desc, digest);
> +	ret = crypto_aead_setkey(data->aead_tfm,
> +				 digest,
> +				 SNAPSHOT_ENCRYPTION_KEY_SIZE);
> +
> +err_free:
> +	kfree(desc);
> +
> +err_rel:
> +	crypto_free_shash(tfm);
> +	return ret;
> +}

Just select CRYPTO_LIB_SHA256, and you can use sha256_init/update/final which
would be much simpler.  Similarly with sha256_data() that is added by the next
patch; you could just call sha256().

- Eric