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.
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 | 52 ++++++++++++-------
2 files changed, 35 insertions(+), 22 deletions(-)
--
2.37.1.455.g008518b4e5-goog
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 <[email protected]>
Signed-off-by: Peter Gonda <[email protected]>
Signed-off-by: Jacky Li <[email protected]>
Reported-by: Alper Gun <[email protected]>
---
.../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);
+ }
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
+ * 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;
+ }
if (sev_es_tmr) {
/*
--
2.37.1.455.g008518b4e5-goog
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 <[email protected]>
Signed-off-by: Jacky Li <[email protected]>
---
drivers/crypto/ccp/sev-dev.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 5bb2ae250d38..fd6bb01eb198 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -233,7 +233,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;
@@ -243,14 +243,15 @@ 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;
+ ret);
+ return ret;
}
nwrite = kernel_write(fp, sev_init_ex_buffer, NV_LENGTH, &offset);
@@ -261,18 +262,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
@@ -287,10 +290,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)
@@ -363,7 +366,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.455.g008518b4e5-goog
Hi Jacky,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on herbert-cryptodev-2.6/master]
[also build test WARNING on herbert-crypto-2.6/master kvm/queue linus/master v5.19 next-20220802]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jacky-Li/Improve-error-handling-during-INIT_EX-file-initialization/20220803-025617
base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20220803/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/3c3fe5b1821e961cbfe1f3724a5256e6e04bbe92
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jacky-Li/Improve-error-handling-during-INIT_EX-file-initialization/20220803-025617
git checkout 3c3fe5b1821e961cbfe1f3724a5256e6e04bbe92
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/crypto/ccp/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from drivers/crypto/ccp/psp-dev.h:13,
from drivers/crypto/ccp/sev-dev.c:30:
drivers/crypto/ccp/sev-dev.c: In function 'sev_write_init_ex_file':
>> drivers/crypto/ccp/sev-dev.c:252:25: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'int' [-Wformat=]
252 | "SEV: could not open file for write, error %ld\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/crypto/ccp/sev-dev.c:251:17: note: in expansion of macro 'dev_err'
251 | dev_err(sev->dev,
| ^~~~~~~
drivers/crypto/ccp/sev-dev.c:252:70: note: format string is defined here
252 | "SEV: could not open file for write, error %ld\n",
| ~~^
| |
| long int
| %d
vim +252 drivers/crypto/ccp/sev-dev.c
3d725965f836a7a David Rientjes 2021-12-07 235
3c3fe5b1821e961 Jacky Li 2022-08-02 236 static int sev_write_init_ex_file(void)
3d725965f836a7a David Rientjes 2021-12-07 237 {
3d725965f836a7a David Rientjes 2021-12-07 238 struct sev_device *sev = psp_master->sev_data;
3d725965f836a7a David Rientjes 2021-12-07 239 struct file *fp;
3d725965f836a7a David Rientjes 2021-12-07 240 loff_t offset = 0;
3d725965f836a7a David Rientjes 2021-12-07 241 ssize_t nwrite;
3d725965f836a7a David Rientjes 2021-12-07 242
3d725965f836a7a David Rientjes 2021-12-07 243 lockdep_assert_held(&sev_cmd_mutex);
3d725965f836a7a David Rientjes 2021-12-07 244
3d725965f836a7a David Rientjes 2021-12-07 245 if (!sev_init_ex_buffer)
3c3fe5b1821e961 Jacky Li 2022-08-02 246 return 0;
3d725965f836a7a David Rientjes 2021-12-07 247
05def5cacfa0bd5 Jacky Li 2022-04-14 248 fp = open_file_as_root(init_ex_path, O_CREAT | O_WRONLY, 0600);
3d725965f836a7a David Rientjes 2021-12-07 249 if (IS_ERR(fp)) {
3c3fe5b1821e961 Jacky Li 2022-08-02 250 int ret = PTR_ERR(fp);
3d725965f836a7a David Rientjes 2021-12-07 251 dev_err(sev->dev,
3d725965f836a7a David Rientjes 2021-12-07 @252 "SEV: could not open file for write, error %ld\n",
3c3fe5b1821e961 Jacky Li 2022-08-02 253 ret);
3c3fe5b1821e961 Jacky Li 2022-08-02 254 return ret;
3d725965f836a7a David Rientjes 2021-12-07 255 }
3d725965f836a7a David Rientjes 2021-12-07 256
3d725965f836a7a David Rientjes 2021-12-07 257 nwrite = kernel_write(fp, sev_init_ex_buffer, NV_LENGTH, &offset);
3d725965f836a7a David Rientjes 2021-12-07 258 vfs_fsync(fp, 0);
3d725965f836a7a David Rientjes 2021-12-07 259 filp_close(fp, NULL);
3d725965f836a7a David Rientjes 2021-12-07 260
3d725965f836a7a David Rientjes 2021-12-07 261 if (nwrite != NV_LENGTH) {
3d725965f836a7a David Rientjes 2021-12-07 262 dev_err(sev->dev,
3d725965f836a7a David Rientjes 2021-12-07 263 "SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
3d725965f836a7a David Rientjes 2021-12-07 264 NV_LENGTH, nwrite);
3c3fe5b1821e961 Jacky Li 2022-08-02 265 return -EIO;
3d725965f836a7a David Rientjes 2021-12-07 266 }
3d725965f836a7a David Rientjes 2021-12-07 267
3d725965f836a7a David Rientjes 2021-12-07 268 dev_dbg(sev->dev, "SEV: write successful to NV file\n");
3c3fe5b1821e961 Jacky Li 2022-08-02 269
3c3fe5b1821e961 Jacky Li 2022-08-02 270 return 0;
3d725965f836a7a David Rientjes 2021-12-07 271 }
3d725965f836a7a David Rientjes 2021-12-07 272
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 <[email protected]>
> Signed-off-by: Peter Gonda <[email protected]>
> Signed-off-by: Jacky Li <[email protected]>
> Reported-by: Alper Gun <[email protected]>
> ---
> .../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) {
> /*
On 8/2/22 13:55, 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 <[email protected]>
> Signed-off-by: Jacky Li <[email protected]>
> ---
> drivers/crypto/ccp/sev-dev.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 5bb2ae250d38..fd6bb01eb198 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -233,7 +233,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;
> @@ -243,14 +243,15 @@ 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);
Please put a blank line after the variable declaration.
> dev_err(sev->dev,
> "SEV: could not open file for write, error %ld\n",
> - PTR_ERR(fp));
> - return;
> + ret);
You'll need to fix the kernel test robot report here.
Thanks,
Tom
> + return ret;
> }
>
> nwrite = kernel_write(fp, sev_init_ex_buffer, NV_LENGTH, &offset);
> @@ -261,18 +262,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
> @@ -287,10 +290,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)
> @@ -363,7 +366,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,
On Wed, Aug 10, 2022 at 1:37 PM Tom Lendacky <[email protected]> wrote:
>
> On 8/2/22 13:55, 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 <[email protected]>
> > Signed-off-by: Jacky Li <[email protected]>
> > ---
> > drivers/crypto/ccp/sev-dev.c | 23 +++++++++++++----------
> > 1 file changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 5bb2ae250d38..fd6bb01eb198 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -233,7 +233,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;
> > @@ -243,14 +243,15 @@ 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);
>
> Please put a blank line after the variable declaration.
>
Will do in the v2. Thanks for the reminder!
> > dev_err(sev->dev,
> > "SEV: could not open file for write, error %ld\n",
> > - PTR_ERR(fp));
> > - return;
> > + ret);
>
> You'll need to fix the kernel test robot report here.
>
> Thanks,
> Tom
>
Will change %ld to %d in v2. Thanks!
Thanks,
Jacky
> > + return ret;
> > }
> >
> > nwrite = kernel_write(fp, sev_init_ex_buffer, NV_LENGTH, &offset);
> > @@ -261,18 +262,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
> > @@ -287,10 +290,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)
> > @@ -363,7 +366,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,
On Wed, Aug 10, 2022 at 1:28 PM Tom Lendacky <[email protected]> wrote:
>
> 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 <[email protected]>
> > Signed-off-by: Peter Gonda <[email protected]>
> > Signed-off-by: Jacky Li <[email protected]>
> > Reported-by: Alper Gun <[email protected]>
> > ---
> > .../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.
>
This function will actually only be called once during the psp
initialization, I will add an info message here in v2 to indicate the
file would be created when missing, thanks!
> > 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..."
>
I'm going to move the sev_read_init_ex_file() to the caller of this
function (i.e. __sev_platform_init_locked) in v2 so that it's less
confusing.
> > + * 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
>
Thanks Tom, this is a great suggestion. I will move the
sev_read_init_ex_file() before the call to the INIT_EX as well as
returning 0 for sev_read_init_ex_file when the file is missing in v2.
Thanks,
Jacky
> > + }
> >
> > if (sev_es_tmr) {
> > /*