| 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: <8dd975d4-e9fc-00eb-d630-cd09410121dc@amd.com>
Date: Wed, 10 Aug 2022 15:28:28 -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 1/2] crypto: ccp - Initialize PSP when reading psp data
file failed
On 8/2/22 13:55, 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:
>
> During 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, it's also needed to skip the sev_read_init_ex_file
> in the second INIT_EX call to prevent the file content from being
> overwritten by the 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>
> ---
> .../virt/kvm/x86/amd-memory-encryption.rst | 5 ++--
> drivers/crypto/ccp/sev-dev.c | 29 +++++++++++++------
> 2 files changed, 22 insertions(+), 12 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 799b476fc3e8..5bb2ae250d38 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -75,6 +75,7 @@ static void *sev_es_tmr;
> */
> #define NV_LENGTH (32 * 1024)
> static void *sev_init_ex_buffer;
> +static bool nv_file_loaded;
>
> static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> {
> @@ -211,18 +212,19 @@ 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_err(sev->dev,
> + "SEV: could not open %s for read, error %d\n",
> + init_ex_path, ret);
> + }
Shouldn't there still be some kind of message if the file is missing? A
typo could result in a file being created that the user isn't expecting.
At least indicating that the file will now possibly be created might be
good. Maybe not here, since this is called multiple times, though.
> 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);
> @@ -417,9 +419,18 @@ static int __sev_init_ex_locked(int *error)
> data.nv_address = __psp_pa(sev_init_ex_buffer);
> data.nv_len = NV_LENGTH;
>
> - ret = sev_read_init_ex_file();
> - if (ret)
> - return ret;
> + /*
> + * The caller of INIT_EX will retry if it fails. Furthermore, if the
This is a little confusing since this function, __sev_init_ex_locked(), is
the caller of INIT_EX. Maybe say "The caller of __sev_init_ex_locked()
will retry..."
> + * failure is due to sev_init_ex_buffer being invalid, the PSP will have
> + * properly initialized the buffer already. Therefore, do not initialize
> + * it a second time.
> + */
> + if (!nv_file_loaded) {
> + ret = sev_read_init_ex_file();
> + if (ret && ret != -ENOENT)
> + return ret;
> + nv_file_loaded = true;
This is really meant to show the INIT_EX has been called, right? Maybe
move this and part of the above comment to just before the call to
INIT_EX. That will make this a bit less confusing.
But, going back to the changes in sev_read_init_ex_file(), couldn't you
just return 0 in the "if (IS_ERR(fp)) {" path if ret == -ENOENT? Then you
don't have to check for it here, too.
Thanks,
Tom
> + }
>
> if (sev_es_tmr) {
> /*
Powered by blists - more mailing lists