| 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
| ||
|
Message-ID: <912e3a42-8084-1c90-6e6e-82308dab28c6@amd.com>
Date: Thu, 25 Aug 2022 14:41:56 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Jacky Li <jackyli@...gle.com>,
Brijesh Singh <brijesh.singh@....com>,
John Allen <john.allen@....com>
Cc: Herbert Xu <herbert@...dor.apana.org.au>,
"David S. Miller" <davem@...emloft.net>,
Marc Orr <marcorr@...gle.com>, Alper Gun <alpergun@...gle.com>,
Peter Gonda <pgonda@...gle.com>, linux-crypto@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] crypto: ccp - Initialize PSP when reading psp data
file failed
On 8/16/22 14:32, Jacky Li wrote:
> Currently the OS fails the PSP initialization when the file specified at
> 'init_ex_path' does not exist or has invalid content. However the SEV
> spec just requires users to allocate 32KB of 0xFF in the file, which can
> be taken care of by the OS easily.
>
> To improve the robustness during the PSP init, leverage the retry
> mechanism and continue the init process:
>
> Before the first INIT_EX call, if the content is invalid or missing,
> continue the process by feeding those contents into PSP instead of
> aborting. PSP will then override it with 32KB 0xFF and return
> SEV_RET_SECURE_DATA_INVALID status code. In the second INIT_EX call,
> this 32KB 0xFF content will then be fed and PSP will write the valid
> data to the file.
>
> In order to do this, sev_read_init_ex_file should only be called once
> for the first INIT_EX call. Calling it again for the second INIT_EX call
> will cause the invalid file content overwriting the valid 32KB 0xFF data
> provided by PSP in the first INIT_EX call.
>
> Co-developed-by: Peter Gonda <pgonda@...gle.com>
> Signed-off-by: Peter Gonda <pgonda@...gle.com>
> Signed-off-by: Jacky Li <jackyli@...gle.com>
> Reported-by: Alper Gun <alpergun@...gle.com>
> ---
> Changelog since v1:
> - Add the message to indicate the possible file creation.
> - Return 0 when the file does not exist in sev_read_init_ex_file().
> - Move sev_read_init_ex_file() before the first call to INIT_EX.
> - Rephrase the last paragraph of the commit message.
>
> .../virt/kvm/x86/amd-memory-encryption.rst | 5 ++-
> drivers/crypto/ccp/sev-dev.c | 36 +++++++++++--------
> 2 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> index 2d307811978c..935aaeb97fe6 100644
> --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> @@ -89,9 +89,8 @@ context. In a typical workflow, this command should be the first command issued.
>
> The firmware can be initialized either by using its own non-volatile storage or
> the OS can manage the NV storage for the firmware using the module parameter
> -``init_ex_path``. The file specified by ``init_ex_path`` must exist. To create
> -a new NV storage file allocate the file with 32KB bytes of 0xFF as required by
> -the SEV spec.
> +``init_ex_path``. If the file specified by ``init_ex_path`` does not exist or
> +is invalid, the OS will create or override the file with output from PSP.
>
> Returns: 0 on success, -negative on error
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 9f588c9728f8..fb7ca45a2f0d 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -211,18 +211,24 @@ static int sev_read_init_ex_file(void)
> if (IS_ERR(fp)) {
> int ret = PTR_ERR(fp);
>
> - dev_err(sev->dev,
> - "SEV: could not open %s for read, error %d\n",
> - init_ex_path, ret);
> + if (ret == -ENOENT) {
> + dev_info(sev->dev,
> + "SEV: %s does not exist and will be created later.\n",
> + init_ex_path);
> + ret = 0;
> + } else {
> + dev_err(sev->dev,
> + "SEV: could not open %s for read, error %d\n",
> + init_ex_path, ret);
> + }
> return ret;
> }
>
> nread = kernel_read(fp, sev_init_ex_buffer, NV_LENGTH, NULL);
> if (nread != NV_LENGTH) {
> - dev_err(sev->dev,
> - "SEV: failed to read %u bytes to non volatile memory area, ret %ld\n",
> + dev_info(sev->dev,
> + "SEV: could not read %u bytes to non volatile memory area, ret %ld\n",
> NV_LENGTH, nread);
> - return -EIO;
> }
>
> dev_dbg(sev->dev, "SEV: read %ld bytes from NV file\n", nread);
> @@ -410,17 +416,12 @@ static int __sev_init_locked(int *error)
> static int __sev_init_ex_locked(int *error)
> {
> struct sev_data_init_ex data;
> - int ret;
>
> memset(&data, 0, sizeof(data));
> data.length = sizeof(data);
> data.nv_address = __psp_pa(sev_init_ex_buffer);
> data.nv_len = NV_LENGTH;
>
> - ret = sev_read_init_ex_file();
> - if (ret)
> - return ret;
> -
> if (sev_es_tmr) {
> /*
> * Do not include the encryption mask on the physical
> @@ -439,7 +440,7 @@ static int __sev_platform_init_locked(int *error)
> {
> struct psp_device *psp = psp_master;
> struct sev_device *sev;
> - int rc, psp_ret = -1;
> + int rc = 0, psp_ret = -1;
This change doesn't look necessary, but not worth having you submit a v3.
Acked-by: Tom Lendacky <thomas.lendacky@....com>
> int (*init_function)(int *error);
>
> if (!psp || !psp->sev_data)
> @@ -450,8 +451,15 @@ static int __sev_platform_init_locked(int *error)
> if (sev->state == SEV_STATE_INIT)
> return 0;
>
> - init_function = sev_init_ex_buffer ? __sev_init_ex_locked :
> - __sev_init_locked;
> + if (sev_init_ex_buffer) {
> + init_function = __sev_init_ex_locked;
> + rc = sev_read_init_ex_file();
> + if (rc)
> + return rc;
> + } else {
> + init_function = __sev_init_locked;
> + }
> +
> rc = init_function(&psp_ret);
> if (rc && psp_ret == SEV_RET_SECURE_DATA_INVALID) {
> /*
Powered by blists - more mailing lists