diff mbox series

[v5,2/7] crypto: ccp: Ensure implicit SEV/SNP init and shutdown in ioctls

Message ID 1d7b31af0eb36d860907c1e89e553e642f3882e0.1740512583.git.ashish.kalra@amd.com
State New
Headers show
Series Move initializing SEV/SNP functionality to KVM | expand

Commit Message

Kalra, Ashish Feb. 25, 2025, 9 p.m. UTC 
From: Ashish Kalra <ashish.kalra@amd.com>

Modify the behavior of implicit SEV initialization in some of the
SEV ioctls to do both SEV initialization and shutdown and add
implicit SNP initialization and shutdown to some of the SNP ioctls
so that the change of SEV/SNP platform initialization not being
done during PSP driver probe time does not break userspace tools
such as sevtool, etc.

Prior to this patch, SEV has always been initialized before these
ioctls as SEV initialization is done as part of PSP module probe,
but now with SEV initialization being moved to KVM module load instead
of PSP driver probe, the implied SEV INIT actually makes sense and gets
used and additionally to maintain SEV platform state consistency
before and after the ioctl SEV shutdown needs to be done after the
firmware call.

It is important to do SEV Shutdown here with the SEV/SNP initialization
moving to KVM, an implicit SEV INIT here as part of the SEV ioctls not
followed with SEV Shutdown will cause SEV to remain in INIT state and
then a future SNP INIT in KVM module load will fail.

Similarly, prior to this patch, SNP has always been initialized before
these ioctls as SNP initialization is done as part of PSP module probe,
therefore, to keep a consistent behavior, SNP init needs to be done
here implicitly as part of these ioctls followed with SNP shutdown
before returning from the ioctl to maintain the consistent platform
state before and after the ioctl.

Suggested-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
 drivers/crypto/ccp/sev-dev.c | 145 +++++++++++++++++++++++++++--------
 1 file changed, 115 insertions(+), 30 deletions(-)

Comments

kernel test robot Feb. 27, 2025, 6:57 p.m. UTC | #1 
Hi Ashish,

kernel test robot noticed the following build warnings:

[auto build test WARNING on herbert-cryptodev-2.6/master]
[also build test WARNING on kvm/queue kvm/next linus/master v6.14-rc4 next-20250227]
[cannot apply to kvm/linux-next]
[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/Ashish-Kalra/crypto-ccp-Move-dev_info-err-messages-for-SEV-SNP-init-and-shutdown/20250226-050640
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
patch link:    https://lore.kernel.org/r/1d7b31af0eb36d860907c1e89e553e642f3882e0.1740512583.git.ashish.kalra%40amd.com
patch subject: [PATCH v5 2/7] crypto: ccp: Ensure implicit SEV/SNP init and shutdown in ioctls
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20250228/202502280243.uLlWONet-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250228/202502280243.uLlWONet-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502280243.uLlWONet-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/crypto/ccp/sev-dev.c:22:
   In file included from include/linux/ccp.h:14:
   In file included from include/linux/scatterlist.h:8:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/crypto/ccp/sev-dev.c:1970:7: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
    1970 |                 if (!writable)
         |                     ^~~~~~~~~
   drivers/crypto/ccp/sev-dev.c:2012:9: note: uninitialized use occurs here
    2012 |         return ret;
         |                ^~~
   drivers/crypto/ccp/sev-dev.c:1970:3: note: remove the 'if' if its condition is always false
    1970 |                 if (!writable)
         |                 ^~~~~~~~~~~~~~
    1971 |                         goto e_free_cert;
         |                         ~~~~~~~~~~~~~~~~
   drivers/crypto/ccp/sev-dev.c:1927:9: note: initialize the variable 'ret' to silence this warning
    1927 |         int ret, error;
         |                ^
         |                 = 0
   4 warnings generated.


vim +1970 drivers/crypto/ccp/sev-dev.c

  1917	
  1918	static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
  1919	{
  1920		struct sev_device *sev = psp_master->sev_data;
  1921		struct sev_user_data_pdh_cert_export input;
  1922		void *pdh_blob = NULL, *cert_blob = NULL;
  1923		struct sev_data_pdh_cert_export data;
  1924		void __user *input_cert_chain_address;
  1925		void __user *input_pdh_cert_address;
  1926		bool shutdown_required = false;
  1927		int ret, error;
  1928	
  1929		if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
  1930			return -EFAULT;
  1931	
  1932		memset(&data, 0, sizeof(data));
  1933	
  1934		/* Userspace wants to query the certificate length. */
  1935		if (!input.pdh_cert_address ||
  1936		    !input.pdh_cert_len ||
  1937		    !input.cert_chain_address)
  1938			goto cmd;
  1939	
  1940		input_pdh_cert_address = (void __user *)input.pdh_cert_address;
  1941		input_cert_chain_address = (void __user *)input.cert_chain_address;
  1942	
  1943		/* Allocate a physically contiguous buffer to store the PDH blob. */
  1944		if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE)
  1945			return -EFAULT;
  1946	
  1947		/* Allocate a physically contiguous buffer to store the cert chain blob. */
  1948		if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE)
  1949			return -EFAULT;
  1950	
  1951		pdh_blob = kzalloc(input.pdh_cert_len, GFP_KERNEL);
  1952		if (!pdh_blob)
  1953			return -ENOMEM;
  1954	
  1955		data.pdh_cert_address = __psp_pa(pdh_blob);
  1956		data.pdh_cert_len = input.pdh_cert_len;
  1957	
  1958		cert_blob = kzalloc(input.cert_chain_len, GFP_KERNEL);
  1959		if (!cert_blob) {
  1960			ret = -ENOMEM;
  1961			goto e_free_pdh;
  1962		}
  1963	
  1964		data.cert_chain_address = __psp_pa(cert_blob);
  1965		data.cert_chain_len = input.cert_chain_len;
  1966	
  1967	cmd:
  1968		/* If platform is not in INIT state then transition it to INIT. */
  1969		if (sev->state != SEV_STATE_INIT) {
> 1970			if (!writable)
  1971				goto e_free_cert;
  1972			ret = __sev_platform_init_locked(&error);
  1973			if (ret) {
  1974				argp->error = SEV_RET_INVALID_PLATFORM_STATE;
  1975				goto e_free_cert;
  1976			}
  1977			shutdown_required = true;
  1978		}
  1979	
  1980		ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
  1981	
  1982		/* If we query the length, FW responded with expected data. */
  1983		input.cert_chain_len = data.cert_chain_len;
  1984		input.pdh_cert_len = data.pdh_cert_len;
  1985	
  1986		if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
  1987			ret = -EFAULT;
  1988			goto e_free_cert;
  1989		}
  1990	
  1991		if (pdh_blob) {
  1992			if (copy_to_user(input_pdh_cert_address,
  1993					 pdh_blob, input.pdh_cert_len)) {
  1994				ret = -EFAULT;
  1995				goto e_free_cert;
  1996			}
  1997		}
  1998	
  1999		if (cert_blob) {
  2000			if (copy_to_user(input_cert_chain_address,
  2001					 cert_blob, input.cert_chain_len))
  2002				ret = -EFAULT;
  2003		}
  2004	
  2005	e_free_cert:
  2006		if (shutdown_required)
  2007			__sev_platform_shutdown_locked(&error);
  2008	
  2009		kfree(cert_blob);
  2010	e_free_pdh:
  2011		kfree(pdh_blob);
  2012		return ret;
  2013	}
  2014
diff mbox series

Patch

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 8962a0dbc66f..14847f1c05fc 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1459,28 +1459,38 @@  static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	int rc;
+	bool shutdown_required = false;
+	int rc, error;
 
 	if (!writable)
 		return -EPERM;
 
 	if (sev->state == SEV_STATE_UNINIT) {
-		rc = __sev_platform_init_locked(&argp->error);
-		if (rc)
+		rc = __sev_platform_init_locked(&error);
+		if (rc) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
 			return rc;
+		}
+		shutdown_required = true;
 	}
 
-	return __sev_do_cmd_locked(cmd, NULL, &argp->error);
+	rc = __sev_do_cmd_locked(cmd, NULL, &argp->error);
+
+	if (shutdown_required)
+		__sev_platform_shutdown_locked(&error);
+
+	return rc;
 }
 
 static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_pek_csr input;
+	bool shutdown_required = false;
 	struct sev_data_pek_csr data;
 	void __user *input_address;
 	void *blob = NULL;
-	int ret;
+	int ret, error;
 
 	if (!writable)
 		return -EPERM;
@@ -1508,9 +1518,12 @@  static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 
 cmd:
 	if (sev->state == SEV_STATE_UNINIT) {
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
+		ret = __sev_platform_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
 			goto e_free_blob;
+		}
+		shutdown_required = true;
 	}
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
@@ -1529,6 +1542,9 @@  static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	}
 
 e_free_blob:
+	if (shutdown_required)
+		__sev_platform_shutdown_locked(&error);
+
 	kfree(blob);
 	return ret;
 }
@@ -1746,8 +1762,9 @@  static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_pek_cert_import input;
 	struct sev_data_pek_cert_import data;
+	bool shutdown_required = false;
 	void *pek_blob, *oca_blob;
-	int ret;
+	int ret, error;
 
 	if (!writable)
 		return -EPERM;
@@ -1776,14 +1793,20 @@  static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 
 	/* If platform is not in INIT state then transition it to INIT */
 	if (sev->state != SEV_STATE_INIT) {
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
+		ret = __sev_platform_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
 			goto e_free_oca;
+		}
+		shutdown_required = true;
 	}
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
 
 e_free_oca:
+	if (shutdown_required)
+		__sev_platform_shutdown_locked(&error);
+
 	kfree(oca_blob);
 e_free_pek:
 	kfree(pek_blob);
@@ -1900,17 +1923,8 @@  static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	struct sev_data_pdh_cert_export data;
 	void __user *input_cert_chain_address;
 	void __user *input_pdh_cert_address;
-	int ret;
-
-	/* If platform is not in INIT state then transition it to INIT. */
-	if (sev->state != SEV_STATE_INIT) {
-		if (!writable)
-			return -EPERM;
-
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
-			return ret;
-	}
+	bool shutdown_required = false;
+	int ret, error;
 
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
@@ -1951,6 +1965,18 @@  static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	data.cert_chain_len = input.cert_chain_len;
 
 cmd:
+	/* If platform is not in INIT state then transition it to INIT. */
+	if (sev->state != SEV_STATE_INIT) {
+		if (!writable)
+			goto e_free_cert;
+		ret = __sev_platform_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			goto e_free_cert;
+		}
+		shutdown_required = true;
+	}
+
 	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
 
 	/* If we query the length, FW responded with expected data. */
@@ -1977,6 +2003,9 @@  static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	}
 
 e_free_cert:
+	if (shutdown_required)
+		__sev_platform_shutdown_locked(&error);
+
 	kfree(cert_blob);
 e_free_pdh:
 	kfree(pdh_blob);
@@ -1986,12 +2015,13 @@  static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 {
 	struct sev_device *sev = psp_master->sev_data;
+	bool shutdown_required = false;
 	struct sev_data_snp_addr buf;
 	struct page *status_page;
+	int ret, error;
 	void *data;
-	int ret;
 
-	if (!sev->snp_initialized || !argp->data)
+	if (!argp->data)
 		return -EINVAL;
 
 	status_page = alloc_page(GFP_KERNEL_ACCOUNT);
@@ -2000,6 +2030,15 @@  static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 
 	data = page_address(status_page);
 
+	if (!sev->snp_initialized) {
+		ret = __sev_snp_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			goto cleanup;
+		}
+		shutdown_required = true;
+	}
+
 	/*
 	 * Firmware expects status page to be in firmware-owned state, otherwise
 	 * it will report firmware error code INVALID_PAGE_STATE (0x1A).
@@ -2028,6 +2067,9 @@  static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 		ret = -EFAULT;
 
 cleanup:
+	if (shutdown_required)
+		__sev_snp_shutdown_locked(&error, false);
+
 	__free_pages(status_page, 0);
 	return ret;
 }
@@ -2036,21 +2078,36 @@  static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_data_snp_commit buf;
+	bool shutdown_required = false;
+	int ret, error;
 
-	if (!sev->snp_initialized)
-		return -EINVAL;
+	if (!sev->snp_initialized) {
+		ret = __sev_snp_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			return ret;
+		}
+		shutdown_required = true;
+	}
 
 	buf.len = sizeof(buf);
 
-	return __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
+
+	if (shutdown_required)
+		__sev_snp_shutdown_locked(&error, false);
+
+	return ret;
 }
 
 static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_snp_config config;
+	bool shutdown_required = false;
+	int ret, error;
 
-	if (!sev->snp_initialized || !argp->data)
+	if (!argp->data)
 		return -EINVAL;
 
 	if (!writable)
@@ -2059,17 +2116,32 @@  static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable
 	if (copy_from_user(&config, (void __user *)argp->data, sizeof(config)))
 		return -EFAULT;
 
-	return __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+	if (!sev->snp_initialized) {
+		ret = __sev_snp_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			return ret;
+		}
+		shutdown_required = true;
+	}
+
+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+
+	if (shutdown_required)
+		__sev_snp_shutdown_locked(&error, false);
+
+	return ret;
 }
 
 static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_snp_vlek_load input;
+	bool shutdown_required = false;
+	int ret, error;
 	void *blob;
-	int ret;
 
-	if (!sev->snp_initialized || !argp->data)
+	if (!argp->data)
 		return -EINVAL;
 
 	if (!writable)
@@ -2088,8 +2160,21 @@  static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
 
 	input.vlek_wrapped_address = __psp_pa(blob);
 
+	if (!sev->snp_initialized) {
+		ret = __sev_snp_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			goto cleanup;
+		}
+		shutdown_required = true;
+	}
+
 	ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
 
+	if (shutdown_required)
+		__sev_snp_shutdown_locked(&error, false);
+
+cleanup:
 	kfree(blob);
 
 	return ret;