[PATCH v2 0/2] Improve error handling during INIT_EX file initialization

[PATCH v2 0/2] Improve error handling during INIT_EX file initialization

[Jacky Li]

Currently the PSP initialization fails when the INIT_EX file is missing
or invalid, while the initialization continues when the OS fails to
write the INIT_EX file. This series handles both cases in a more robust
way by resolving the file read error as well as throwing the write error
to the caller.
---
Changelog since v1:
- refactor around __sev_init_ex_locked() and fix format.

Jacky Li (2):
  crypto: ccp - Initialize PSP when reading psp data file failed
  crypto: ccp - Fail the PSP initialization when writing psp data file
    failed

 .../virt/kvm/x86/amd-memory-encryption.rst    |  5 +-
 drivers/crypto/ccp/sev-dev.c                  | 62 +++++++++++--------
 2 files changed, 39 insertions(+), 28 deletions(-)

-- 
2.37.1.595.g718a3a8f04-goog

[PATCH v2 1/2] crypto: ccp - Initialize PSP when reading psp data file failed

[Jacky Li]

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@google.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
Signed-off-by: Jacky Li <jackyli@google.com>
Reported-by: Alper Gun <alpergun@google.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;
 	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) {
 		/*
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH v2 1/2] crypto: ccp - Initialize PSP when reading psp data file failed

[David Rientjes]

On Tue, 16 Aug 2022, 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@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Signed-off-by: Jacky Li <jackyli@google.com>
> Reported-by: Alper Gun <alpergun@google.com>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH v2 1/2] crypto: ccp - Initialize PSP when reading psp data file failed

[Tom Lendacky]

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@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Signed-off-by: Jacky Li <jackyli@google.com>
> Reported-by: Alper Gun <alpergun@google.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@amd.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) {
>   		/*

[PATCH v2 2/2] crypto: ccp - Fail the PSP initialization when writing psp data file failed

[Jacky Li]

Currently the OS continues the PSP initialization when there is a write
failure to the init_ex_file. Therefore, the userspace would be told that
SEV is properly INIT'd even though the psp data file is not updated.
This is problematic because later when asked for the SEV data, the OS
won't be able to provide it.

Fixes: 3d725965f836 ("crypto: ccp - Add SEV_INIT_EX support")
Reported-by: Peter Gonda <pgonda@google.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Jacky Li <jackyli@google.com>
---
Changelog since v1:
- Add a blank line after the variable declaration.
- Fix the string format of the error code.

 drivers/crypto/ccp/sev-dev.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index fb7ca45a2f0d..ab1f76549ef8 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -237,7 +237,7 @@ static int sev_read_init_ex_file(void)
 	return 0;
 }
 
-static void sev_write_init_ex_file(void)
+static int sev_write_init_ex_file(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct file *fp;
@@ -247,14 +247,16 @@ static void sev_write_init_ex_file(void)
 	lockdep_assert_held(&sev_cmd_mutex);
 
 	if (!sev_init_ex_buffer)
-		return;
+		return 0;
 
 	fp = open_file_as_root(init_ex_path, O_CREAT | O_WRONLY, 0600);
 	if (IS_ERR(fp)) {
+		int ret = PTR_ERR(fp);
+
 		dev_err(sev->dev,
-			"SEV: could not open file for write, error %ld\n",
-			PTR_ERR(fp));
-		return;
+			"SEV: could not open file for write, error %d\n",
+			ret);
+		return ret;
 	}
 
 	nwrite = kernel_write(fp, sev_init_ex_buffer, NV_LENGTH, &offset);
@@ -265,18 +267,20 @@ static void sev_write_init_ex_file(void)
 		dev_err(sev->dev,
 			"SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
 			NV_LENGTH, nwrite);
-		return;
+		return -EIO;
 	}
 
 	dev_dbg(sev->dev, "SEV: write successful to NV file\n");
+
+	return 0;
 }
 
-static void sev_write_init_ex_file_if_required(int cmd_id)
+static int sev_write_init_ex_file_if_required(int cmd_id)
 {
 	lockdep_assert_held(&sev_cmd_mutex);
 
 	if (!sev_init_ex_buffer)
-		return;
+		return 0;
 
 	/*
 	 * Only a few platform commands modify the SPI/NV area, but none of the
@@ -291,10 +295,10 @@ static void sev_write_init_ex_file_if_required(int cmd_id)
 	case SEV_CMD_PEK_GEN:
 		break;
 	default:
-		return;
+		return 0;
 	}
 
-	sev_write_init_ex_file();
+	return sev_write_init_ex_file();
 }
 
 static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
@@ -367,7 +371,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 			cmd, reg & PSP_CMDRESP_ERR_MASK);
 		ret = -EIO;
 	} else {
-		sev_write_init_ex_file_if_required(cmd);
+		ret = sev_write_init_ex_file_if_required(cmd);
 	}
 
 	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH v2 2/2] crypto: ccp - Fail the PSP initialization when writing psp data file failed

[David Rientjes]

On Tue, 16 Aug 2022, Jacky Li wrote:

> Currently the OS continues the PSP initialization when there is a write
> failure to the init_ex_file. Therefore, the userspace would be told that
> SEV is properly INIT'd even though the psp data file is not updated.
> This is problematic because later when asked for the SEV data, the OS
> won't be able to provide it.
> 
> Fixes: 3d725965f836 ("crypto: ccp - Add SEV_INIT_EX support")
> Reported-by: Peter Gonda <pgonda@google.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Jacky Li <jackyli@google.com>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH v2 2/2] crypto: ccp - Fail the PSP initialization when writing psp data file failed

[Tom Lendacky]

On 8/16/22 14:32, Jacky Li wrote:
> Currently the OS continues the PSP initialization when there is a write
> failure to the init_ex_file. Therefore, the userspace would be told that
> SEV is properly INIT'd even though the psp data file is not updated.
> This is problematic because later when asked for the SEV data, the OS
> won't be able to provide it.
> 
> Fixes: 3d725965f836 ("crypto: ccp - Add SEV_INIT_EX support")
> Reported-by: Peter Gonda <pgonda@google.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Jacky Li <jackyli@google.com>

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
> Changelog since v1:
> - Add a blank line after the variable declaration.
> - Fix the string format of the error code.
> 
>   drivers/crypto/ccp/sev-dev.c | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index fb7ca45a2f0d..ab1f76549ef8 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -237,7 +237,7 @@ static int sev_read_init_ex_file(void)
>   	return 0;
>   }
>   
> -static void sev_write_init_ex_file(void)
> +static int sev_write_init_ex_file(void)
>   {
>   	struct sev_device *sev = psp_master->sev_data;
>   	struct file *fp;
> @@ -247,14 +247,16 @@ static void sev_write_init_ex_file(void)
>   	lockdep_assert_held(&sev_cmd_mutex);
>   
>   	if (!sev_init_ex_buffer)
> -		return;
> +		return 0;
>   
>   	fp = open_file_as_root(init_ex_path, O_CREAT | O_WRONLY, 0600);
>   	if (IS_ERR(fp)) {
> +		int ret = PTR_ERR(fp);
> +
>   		dev_err(sev->dev,
> -			"SEV: could not open file for write, error %ld\n",
> -			PTR_ERR(fp));
> -		return;
> +			"SEV: could not open file for write, error %d\n",
> +			ret);
> +		return ret;
>   	}
>   
>   	nwrite = kernel_write(fp, sev_init_ex_buffer, NV_LENGTH, &offset);
> @@ -265,18 +267,20 @@ static void sev_write_init_ex_file(void)
>   		dev_err(sev->dev,
>   			"SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
>   			NV_LENGTH, nwrite);
> -		return;
> +		return -EIO;
>   	}
>   
>   	dev_dbg(sev->dev, "SEV: write successful to NV file\n");
> +
> +	return 0;
>   }
>   
> -static void sev_write_init_ex_file_if_required(int cmd_id)
> +static int sev_write_init_ex_file_if_required(int cmd_id)
>   {
>   	lockdep_assert_held(&sev_cmd_mutex);
>   
>   	if (!sev_init_ex_buffer)
> -		return;
> +		return 0;
>   
>   	/*
>   	 * Only a few platform commands modify the SPI/NV area, but none of the
> @@ -291,10 +295,10 @@ static void sev_write_init_ex_file_if_required(int cmd_id)
>   	case SEV_CMD_PEK_GEN:
>   		break;
>   	default:
> -		return;
> +		return 0;
>   	}
>   
> -	sev_write_init_ex_file();
> +	return sev_write_init_ex_file();
>   }
>   
>   static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> @@ -367,7 +371,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>   			cmd, reg & PSP_CMDRESP_ERR_MASK);
>   		ret = -EIO;
>   	} else {
> -		sev_write_init_ex_file_if_required(cmd);
> +		ret = sev_write_init_ex_file_if_required(cmd);
>   	}
>   
>   	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,

Re: [PATCH v2 0/2] Improve error handling during INIT_EX file initialization

[Herbert Xu]

On Tue, Aug 16, 2022 at 07:32:07PM +0000, Jacky Li wrote:
> Currently the PSP initialization fails when the INIT_EX file is missing
> or invalid, while the initialization continues when the OS fails to
> write the INIT_EX file. This series handles both cases in a more robust
> way by resolving the file read error as well as throwing the write error
> to the caller.
> ---
> Changelog since v1:
> - refactor around __sev_init_ex_locked() and fix format.
> 
> Jacky Li (2):
>   crypto: ccp - Initialize PSP when reading psp data file failed
>   crypto: ccp - Fail the PSP initialization when writing psp data file
>     failed
> 
>  .../virt/kvm/x86/amd-memory-encryption.rst    |  5 +-
>  drivers/crypto/ccp/sev-dev.c                  | 62 +++++++++++--------
>  2 files changed, 39 insertions(+), 28 deletions(-)
> 
> -- 
> 2.37.1.595.g718a3a8f04-goog

All applied.  Thanks. 
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt