diff mbox series

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

Message ID 20220802185534.735338-3-jackyli@google.com
State Superseded
Headers show
Series Improve error handling during INIT_EX file initialization | expand

Commit Message

Jacky Li Aug. 2, 2022, 6:55 p.m. UTC 
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>
Signed-off-by: Jacky Li <jackyli@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Comments

kernel test robot Aug. 3, 2022, 2:19 a.m. UTC | #1 
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/202208031012.z1rYKkYA-lkp@intel.com/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 <lkp@intel.com>

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
Tom Lendacky Aug. 10, 2022, 8:37 p.m. UTC | #2 
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 <pgonda@google.com>
> Signed-off-by: Jacky Li <jackyli@google.com>
> ---
>   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,
Jacky Li Aug. 16, 2022, 7:30 p.m. UTC | #3 
On Wed, Aug 10, 2022 at 1:37 PM Tom Lendacky <thomas.lendacky@amd.com> 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 <pgonda@google.com>
> > Signed-off-by: Jacky Li <jackyli@google.com>
> > ---
> >   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,
diff mbox series

Patch

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,