* [Part2 PATCH v5.1 12.2/31] crypto: ccp: Define SEV userspace ioctl and command id
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
@ 2017-10-07 1:06 ` Brijesh Singh
2017-10-07 14:20 ` Borislav Petkov
2017-10-11 16:46 ` [Part2 PATCH v5.2 12.1/31] " Brijesh Singh
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command Brijesh Singh
` (8 subsequent siblings)
9 siblings, 2 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-07 1:06 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
Add a include file which defines the ioctl and command id used for
issuing SEV platform management specific commands.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
include/uapi/linux/psp-sev.h | 115 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 115 insertions(+)
create mode 100644 include/uapi/linux/psp-sev.h
diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
new file mode 100644
index 000000000000..a385bf2b8d2a
--- /dev/null
+++ b/include/uapi/linux/psp-sev.h
@@ -0,0 +1,115 @@
+/*
+ * Userspace interface for AMD Secure Encrypted Virtualization (SEV)
+ * platform management commands.
+ *
+ * Copyright (C) 2016-2017 Advanced Micro Devices, Inc.
+ *
+ * Author: Brijesh Singh <brijesh.singh@amd.com>
+ *
+ * SEV spec 0.14 is available at:
+ * http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __PSP_SEV_USER_H__
+#define __PSP_SEV_USER_H__
+
+#include <linux/types.h>
+
+/**
+ * SEV platform commands
+ */
+enum {
+ SEV_FACTORY_RESET = 0,
+ SEV_PLATFORM_STATUS,
+ SEV_PEK_GEN,
+ SEV_PEK_CSR,
+ SEV_PDH_GEN,
+ SEV_PDH_CERT_EXPORT,
+ SEV_PEK_CERT_IMPORT,
+
+ SEV_MAX,
+};
+
+/**
+ * struct sev_user_data_status - PLATFORM_STATUS command parameters
+ *
+ * @major: major API version
+ * @minor: minor API version
+ * @state: platform state
+ * @owner: self-owned or externally owned
+ * @config: platform config flags
+ * @build: firmware build id for API version
+ * @guest_count: number of active guests
+ */
+struct sev_user_data_status {
+ __u8 api_major; /* Out */
+ __u8 api_minor; /* Out */
+ __u8 state; /* Out */
+ __u8 owner; /* Out */
+ __u32 config; /* Out */
+ __u8 build; /* Out */
+ __u32 guest_count; /* Out */
+};
+
+/**
+ * struct sev_user_data_pek_csr - PEK_CSR command parameters
+ *
+ * @address: PEK certificate chain
+ * @length: length of certificate
+ */
+struct sev_user_data_pek_csr {
+ __u64 address; /* In */
+ __u32 length; /* In/Out */
+};
+
+/**
+ * struct sev_user_data_cert_import - PEK_CERT_IMPORT command parameters
+ *
+ * @pek_address: PEK certificate chain
+ * @pek_len: length of PEK certificate
+ * @oca_address: OCA certificate chain
+ * @oca_len: length of OCA certificate
+ */
+struct sev_user_data_pek_cert_import {
+ __u64 pek_cert_address; /* In */
+ __u32 pek_cert_len; /* In */
+ __u64 oca_cert_address; /* In */
+ __u32 oca_cert_len; /* In */
+};
+
+/**
+ * struct sev_user_data_pdh_cert_export - PDH_CERT_EXPORT command parameters
+ *
+ * @pdh_address: PDH certificate address
+ * @pdh_len: length of PDH certificate
+ * @cert_chain_address: PDH certificate chain
+ * @cert_chain_len: length of PDH certificate chain
+ */
+struct sev_user_data_pdh_cert_export {
+ __u64 pdh_cert_address; /* In */
+ __u32 pdh_cert_len; /* In/Out */
+ __u64 cert_chain_address; /* In */
+ __u32 cert_chain_len; /* In/Out */
+};
+
+/**
+ * struct sev_issue_cmd - SEV ioctl parameters
+ *
+ * @cmd: SEV commands to execute
+ * @opaque: pointer to the command structure
+ * @error: SEV FW return code on failure
+ */
+struct sev_issue_cmd {
+ __u32 cmd; /* In */
+ __u64 data; /* In */
+ __u32 error; /* Out */
+};
+
+#define SEV_IOC_TYPE 'S'
+#define SEV_ISSUE_CMD _IOWR(SEV_IOC_TYPE, 0x0, struct sev_issue_cmd)
+
+#endif /* __PSP_USER_SEV_H */
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.2/31] crypto: ccp: Define SEV userspace ioctl and command id
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.2/31] crypto: ccp: Define SEV userspace ioctl and command id Brijesh Singh
@ 2017-10-07 14:20 ` Borislav Petkov
2017-10-08 21:18 ` Brijesh Singh
2017-10-11 16:46 ` [Part2 PATCH v5.2 12.1/31] " Brijesh Singh
1 sibling, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-07 14:20 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Fri, Oct 06, 2017 at 08:06:00PM -0500, Brijesh Singh wrote:
> Add a include file which defines the ioctl and command id used for
> issuing SEV platform management specific commands.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> include/uapi/linux/psp-sev.h | 115 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 115 insertions(+)
> create mode 100644 include/uapi/linux/psp-sev.h
First of all, thanks for splitting the patch - it is much easier to
review this way.
Then, this patch should be 12.1, i.e., the first of the split because otherwise
the previous one - which should be the next - fails building due to
drivers/crypto/ccp/psp-dev.c:26:32: fatal error: uapi/linux/psp-sev.h: No such file or directory
#include <uapi/linux/psp-sev.h>
^
Just swap them in their order.
Also, those SEV commands should be __packed, see below.
With that addressed:
Reviewed-by: Borislav Petkov <bp@suse.de>
---
diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
index a385bf2b8d2a..b63e116f18c1 100644
--- a/include/uapi/linux/psp-sev.h
+++ b/include/uapi/linux/psp-sev.h
@@ -53,7 +53,7 @@ struct sev_user_data_status {
__u32 config; /* Out */
__u8 build; /* Out */
__u32 guest_count; /* Out */
-};
+} __packed;
/**
* struct sev_user_data_pek_csr - PEK_CSR command parameters
@@ -64,7 +64,7 @@ struct sev_user_data_status {
struct sev_user_data_pek_csr {
__u64 address; /* In */
__u32 length; /* In/Out */
-};
+} __packed;
/**
* struct sev_user_data_cert_import - PEK_CERT_IMPORT command parameters
@@ -79,7 +79,7 @@ struct sev_user_data_pek_cert_import {
__u32 pek_cert_len; /* In */
__u64 oca_cert_address; /* In */
__u32 oca_cert_len; /* In */
-};
+} __packed;
/**
* struct sev_user_data_pdh_cert_export - PDH_CERT_EXPORT command parameters
@@ -94,7 +94,7 @@ struct sev_user_data_pdh_cert_export {
__u32 pdh_cert_len; /* In/Out */
__u64 cert_chain_address; /* In */
__u32 cert_chain_len; /* In/Out */
-};
+} __packed;
/**
* struct sev_issue_cmd - SEV ioctl parameters
@@ -107,7 +107,7 @@ struct sev_issue_cmd {
__u32 cmd; /* In */
__u64 data; /* In */
__u32 error; /* Out */
-};
+} __packed;
#define SEV_IOC_TYPE 'S'
#define SEV_ISSUE_CMD _IOWR(SEV_IOC_TYPE, 0x0, struct sev_issue_cmd)
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.2/31] crypto: ccp: Define SEV userspace ioctl and command id
2017-10-07 14:20 ` Borislav Petkov
@ 2017-10-08 21:18 ` Brijesh Singh
0 siblings, 0 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-08 21:18 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/7/17 9:20 AM, Borislav Petkov wrote:
> On Fri, Oct 06, 2017 at 08:06:00PM -0500, Brijesh Singh wrote:
>> Add a include file which defines the ioctl and command id used for
>> issuing SEV platform management specific commands.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: Gary Hook <gary.hook@amd.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: linux-crypto@vger.kernel.org
>> Cc: kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> include/uapi/linux/psp-sev.h | 115 +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 115 insertions(+)
>> create mode 100644 include/uapi/linux/psp-sev.h
> First of all, thanks for splitting the patch - it is much easier to
> review this way.
>
> Then, this patch should be 12.1, i.e., the first of the split because otherwise
> the previous one - which should be the next - fails building due to
>
> drivers/crypto/ccp/psp-dev.c:26:32: fatal error: uapi/linux/psp-sev.h: No such file or directory
> #include <uapi/linux/psp-sev.h>
> ^
> Just swap them in their order.
Ah, yes I will swap the order in next submission. thanks
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Part2 PATCH v5.2 12.1/31] crypto: ccp: Define SEV userspace ioctl and command id
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.2/31] crypto: ccp: Define SEV userspace ioctl and command id Brijesh Singh
2017-10-07 14:20 ` Borislav Petkov
@ 2017-10-11 16:46 ` Brijesh Singh
2017-10-12 13:27 ` Borislav Petkov
1 sibling, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-11 16:46 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
Add a include file which defines the ioctl and command id used for
issuing SEV platform management specific commands.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Improvements-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Reviewed-by: Borislav Petkov <bp@suse.de>
---
Make it as the first patch in the series (changed from 12.2/31 -> 12.1/31)
Changes since v5.1:
* add __packed improvement from Boris
The full tree is available at:
repo: https://github.com/codomania/kvm
branch: sev-v5-p2+fixes
include/uapi/linux/psp-sev.h | 115 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 115 insertions(+)
create mode 100644 include/uapi/linux/psp-sev.h
diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
new file mode 100644
index 000000000000..b63e116f18c1
--- /dev/null
+++ b/include/uapi/linux/psp-sev.h
@@ -0,0 +1,115 @@
+/*
+ * Userspace interface for AMD Secure Encrypted Virtualization (SEV)
+ * platform management commands.
+ *
+ * Copyright (C) 2016-2017 Advanced Micro Devices, Inc.
+ *
+ * Author: Brijesh Singh <brijesh.singh@amd.com>
+ *
+ * SEV spec 0.14 is available at:
+ * http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __PSP_SEV_USER_H__
+#define __PSP_SEV_USER_H__
+
+#include <linux/types.h>
+
+/**
+ * SEV platform commands
+ */
+enum {
+ SEV_FACTORY_RESET = 0,
+ SEV_PLATFORM_STATUS,
+ SEV_PEK_GEN,
+ SEV_PEK_CSR,
+ SEV_PDH_GEN,
+ SEV_PDH_CERT_EXPORT,
+ SEV_PEK_CERT_IMPORT,
+
+ SEV_MAX,
+};
+
+/**
+ * struct sev_user_data_status - PLATFORM_STATUS command parameters
+ *
+ * @major: major API version
+ * @minor: minor API version
+ * @state: platform state
+ * @owner: self-owned or externally owned
+ * @config: platform config flags
+ * @build: firmware build id for API version
+ * @guest_count: number of active guests
+ */
+struct sev_user_data_status {
+ __u8 api_major; /* Out */
+ __u8 api_minor; /* Out */
+ __u8 state; /* Out */
+ __u8 owner; /* Out */
+ __u32 config; /* Out */
+ __u8 build; /* Out */
+ __u32 guest_count; /* Out */
+} __packed;
+
+/**
+ * struct sev_user_data_pek_csr - PEK_CSR command parameters
+ *
+ * @address: PEK certificate chain
+ * @length: length of certificate
+ */
+struct sev_user_data_pek_csr {
+ __u64 address; /* In */
+ __u32 length; /* In/Out */
+} __packed;
+
+/**
+ * struct sev_user_data_cert_import - PEK_CERT_IMPORT command parameters
+ *
+ * @pek_address: PEK certificate chain
+ * @pek_len: length of PEK certificate
+ * @oca_address: OCA certificate chain
+ * @oca_len: length of OCA certificate
+ */
+struct sev_user_data_pek_cert_import {
+ __u64 pek_cert_address; /* In */
+ __u32 pek_cert_len; /* In */
+ __u64 oca_cert_address; /* In */
+ __u32 oca_cert_len; /* In */
+} __packed;
+
+/**
+ * struct sev_user_data_pdh_cert_export - PDH_CERT_EXPORT command parameters
+ *
+ * @pdh_address: PDH certificate address
+ * @pdh_len: length of PDH certificate
+ * @cert_chain_address: PDH certificate chain
+ * @cert_chain_len: length of PDH certificate chain
+ */
+struct sev_user_data_pdh_cert_export {
+ __u64 pdh_cert_address; /* In */
+ __u32 pdh_cert_len; /* In/Out */
+ __u64 cert_chain_address; /* In */
+ __u32 cert_chain_len; /* In/Out */
+} __packed;
+
+/**
+ * struct sev_issue_cmd - SEV ioctl parameters
+ *
+ * @cmd: SEV commands to execute
+ * @opaque: pointer to the command structure
+ * @error: SEV FW return code on failure
+ */
+struct sev_issue_cmd {
+ __u32 cmd; /* In */
+ __u64 data; /* In */
+ __u32 error; /* Out */
+} __packed;
+
+#define SEV_IOC_TYPE 'S'
+#define SEV_ISSUE_CMD _IOWR(SEV_IOC_TYPE, 0x0, struct sev_issue_cmd)
+
+#endif /* __PSP_USER_SEV_H */
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.2 12.1/31] crypto: ccp: Define SEV userspace ioctl and command id
2017-10-11 16:46 ` [Part2 PATCH v5.2 12.1/31] " Brijesh Singh
@ 2017-10-12 13:27 ` Borislav Petkov
2017-10-12 14:18 ` Brijesh Singh
0 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 13:27 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Wed, Oct 11, 2017 at 11:46:05AM -0500, Brijesh Singh wrote:
> Add a include file which defines the ioctl and command id used for
> issuing SEV platform management specific commands.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Improvements-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Reviewed-by: Borislav Petkov <bp@suse.de>
> ---
> Make it as the first patch in the series (changed from 12.2/31 -> 12.1/31)
>
> Changes since v5.1:
> * add __packed improvement from Boris
...
> +/**
> + * struct sev_user_data_status - PLATFORM_STATUS command parameters
> + *
> + * @major: major API version
> + * @minor: minor API version
> + * @state: platform state
> + * @owner: self-owned or externally owned
> + * @config: platform config flags
> + * @build: firmware build id for API version
> + * @guest_count: number of active guests
> + */
> +struct sev_user_data_status {
> + __u8 api_major; /* Out */
> + __u8 api_minor; /* Out */
> + __u8 state; /* Out */
> + __u8 owner; /* Out */
> + __u32 config; /* Out */
> + __u8 build; /* Out */
> + __u32 guest_count; /* Out */
> +} __packed;
> +
After yesterday's discussion about the sev_data_status layout, that
struct is not needed anymore, right?
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.2 12.1/31] crypto: ccp: Define SEV userspace ioctl and command id
2017-10-12 13:27 ` Borislav Petkov
@ 2017-10-12 14:18 ` Brijesh Singh
0 siblings, 0 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-12 14:18 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/12/2017 08:27 AM, Borislav Petkov wrote:
...
>
>> +/**
>> + * struct sev_user_data_status - PLATFORM_STATUS command parameters
>> + *
>> + * @major: major API version
>> + * @minor: minor API version
>> + * @state: platform state
>> + * @owner: self-owned or externally owned
>> + * @config: platform config flags
>> + * @build: firmware build id for API version
>> + * @guest_count: number of active guests
>> + */
>> +struct sev_user_data_status {
>> + __u8 api_major; /* Out */
>> + __u8 api_minor; /* Out */
>> + __u8 state; /* Out */
>> + __u8 owner; /* Out */
>> + __u32 config; /* Out */
>> + __u8 build; /* Out */
>> + __u32 guest_count; /* Out */
>> +} __packed;
>> +
>
> After yesterday's discussion about the sev_data_status layout, that
> struct is not needed anymore, right?
>
Yes, we no longer need that structure.
-Brijesh
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Part2 PATCH v5.1 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.2/31] crypto: ccp: Define SEV userspace ioctl and command id Brijesh Singh
@ 2017-10-07 1:06 ` Brijesh Singh
2017-10-11 14:32 ` Borislav Petkov
2017-10-11 16:55 ` [Part2 PATCH v5.2 " Brijesh Singh
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS " Brijesh Singh
` (7 subsequent siblings)
9 siblings, 2 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-07 1:06 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
The SEV_FACTORY_RESET command can be used by the platform owner to
reset the non-volatile SEV related data. The command is defined in
SEV spec section 5.4
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
drivers/crypto/ccp/psp-dev.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e9b776c3acb2..94a08c371bda 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -179,7 +179,34 @@ static int sev_handle_cmd(int cmd, void *data, int *psp_ret)
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
- return -ENOTTY;
+ void __user *argp = (void __user *)arg;
+ struct sev_issue_cmd input;
+ int ret = -EFAULT;
+
+ if (ioctl != SEV_ISSUE_CMD)
+ return -EINVAL;
+
+ if (copy_from_user(&input, argp, sizeof(struct sev_issue_cmd)))
+ return -EFAULT;
+
+ if (input.cmd > SEV_CMD_MAX)
+ return -EINVAL;
+
+ switch (input.cmd) {
+
+ case SEV_FACTORY_RESET: {
+ ret = sev_handle_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
+ break;
+ }
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
+ ret = -EFAULT;
+
+ return ret;
}
const struct file_operations sev_fops = {
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command Brijesh Singh
@ 2017-10-11 14:32 ` Borislav Petkov
2017-10-11 16:55 ` [Part2 PATCH v5.2 " Brijesh Singh
1 sibling, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-11 14:32 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Fri, Oct 06, 2017 at 08:06:01PM -0500, Brijesh Singh wrote:
> The SEV_FACTORY_RESET command can be used by the platform owner to
> reset the non-volatile SEV related data. The command is defined in
> SEV spec section 5.4
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> drivers/crypto/ccp/psp-dev.c | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
Some fixes ontop, like, for example, if you hit the default: label of
the switch due to input.cmd being one of the ones in the holes in enum
sev_cmd, you don't need to copy_to_user() in the end.
---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d3a50f1f737e..ed5a7404b5a5 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -192,19 +192,19 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
return -EINVAL;
switch (input.cmd) {
-
- case SEV_FACTORY_RESET: {
- ret = sev_handle_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
+ case SEV_FACTORY_RESET:
+ ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
break;
- }
+
default:
ret = -EINVAL;
- break;
+ goto out;
}
if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
ret = -EFAULT;
+out:
return ret;
}
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Part2 PATCH v5.2 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command Brijesh Singh
2017-10-11 14:32 ` Borislav Petkov
@ 2017-10-11 16:55 ` Brijesh Singh
2017-10-12 14:13 ` Borislav Petkov
1 sibling, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-11 16:55 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
The SEV_FACTORY_RESET command can be used by the platform owner to
reset the non-volatile SEV related data. The command is defined in
SEV spec section 5.4
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Improvements-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
Changes since v5.1:
* rename sev_handle_cmd -> sev_do_cmd (from Boris)
* skip copy_to_user when invalid cmd id is passed (from Boris)
* use SEV_MAX instead of SEV_CMD_MAX to check for invalid command
drivers/crypto/ccp/psp-dev.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 175cb3c3b8ef..a9c885a39910 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -179,7 +179,34 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
- return -ENOTTY;
+ void __user *argp = (void __user *)arg;
+ struct sev_issue_cmd input;
+ int ret = -EFAULT;
+
+ if (ioctl != SEV_ISSUE_CMD)
+ return -EINVAL;
+
+ if (copy_from_user(&input, argp, sizeof(struct sev_issue_cmd)))
+ return -EFAULT;
+
+ if (input.cmd > SEV_MAX)
+ return -EINVAL;
+
+ switch (input.cmd) {
+
+ case SEV_FACTORY_RESET: {
+ ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
+ break;
+ }
+ default:
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
+ ret = -EFAULT;
+out:
+ return ret;
}
static const struct file_operations sev_fops = {
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.2 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command
2017-10-11 16:55 ` [Part2 PATCH v5.2 " Brijesh Singh
@ 2017-10-12 14:13 ` Borislav Petkov
0 siblings, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 14:13 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Wed, Oct 11, 2017 at 11:55:21AM -0500, Brijesh Singh wrote:
> The SEV_FACTORY_RESET command can be used by the platform owner to
> reset the non-volatile SEV related data. The command is defined in
> SEV spec section 5.4
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Improvements-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>
> Changes since v5.1:
> * rename sev_handle_cmd -> sev_do_cmd (from Boris)
> * skip copy_to_user when invalid cmd id is passed (from Boris)
> * use SEV_MAX instead of SEV_CMD_MAX to check for invalid command
>
> drivers/crypto/ccp/psp-dev.c | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 175cb3c3b8ef..a9c885a39910 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -179,7 +179,34 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
>
> static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> {
> - return -ENOTTY;
> + void __user *argp = (void __user *)arg;
> + struct sev_issue_cmd input;
> + int ret = -EFAULT;
> +
> + if (ioctl != SEV_ISSUE_CMD)
> + return -EINVAL;
> +
> + if (copy_from_user(&input, argp, sizeof(struct sev_issue_cmd)))
> + return -EFAULT;
> +
> + if (input.cmd > SEV_MAX)
> + return -EINVAL;
> +
> + switch (input.cmd) {
> +
> + case SEV_FACTORY_RESET: {
> + ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
> + break;
> + }
Those curly braces are still here. Just use my diff I sent you by saving
the mail to a file and doing
$ patch -p1 --dry-run -i /tmp/boris.mail
ontop of this patch. If it applies ok, remove the --dry-run.
Then you can do changes ontop. This way you won't miss changes I've sent
you.
Thx.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.2/31] crypto: ccp: Define SEV userspace ioctl and command id Brijesh Singh
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command Brijesh Singh
@ 2017-10-07 1:06 ` Brijesh Singh
2017-10-11 17:01 ` [Part2 PATCH v5.2 " Brijesh Singh
2017-10-11 17:02 ` [Part2 PATCH v5.1 " Borislav Petkov
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN " Brijesh Singh
` (6 subsequent siblings)
9 siblings, 2 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-07 1:06 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
The SEV_PLATFORM_STATUS command can be used by the platform owner to
get the current status of the platform. The command is defined in
SEV spec section 5.5.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
drivers/crypto/ccp/psp-dev.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 94a08c371bda..d68303a06464 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -177,6 +177,36 @@ static int sev_handle_cmd(int cmd, void *data, int *psp_ret)
return ret;
}
+static int sev_ioctl_platform_status(struct sev_issue_cmd *argp)
+{
+ struct sev_user_data_status out;
+ struct sev_data_status *data;
+ int ret;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error);
+ if (ret)
+ goto e_free;
+
+ out.api_major = data->api_major;
+ out.api_minor = data->api_minor;
+ out.state = data->state;
+ out.owner = data->owner;
+ out.config = data->config;
+ out.build = data->build;
+ out.guest_count = data->guest_count;
+ if (copy_to_user((void __user *)(uintptr_t) argp->data,
+ &out, sizeof(struct sev_user_data_status)))
+ ret = -EFAULT;
+
+e_free:
+ kfree(data);
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
void __user *argp = (void __user *)arg;
@@ -198,6 +228,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_handle_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
break;
}
+ case SEV_PLATFORM_STATUS: {
+ ret = sev_ioctl_platform_status(&input);
+ break;
+ }
default:
ret = -EINVAL;
break;
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* [Part2 PATCH v5.2 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS " Brijesh Singh
@ 2017-10-11 17:01 ` Brijesh Singh
2017-10-11 17:02 ` [Part2 PATCH v5.1 " Borislav Petkov
1 sibling, 0 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-11 17:01 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
The SEV_PLATFORM_STATUS command can be used by the platform owner to
get the current status of the platform. The command is defined in
SEV spec section 5.5.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
Changes since v5.1:
* rename sev_handle_cmd -> sev_do_cmd
drivers/crypto/ccp/psp-dev.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index a9c885a39910..1f4925cf9ae5 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -177,6 +177,36 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
return ret;
}
+static int sev_ioctl_platform_status(struct sev_issue_cmd *argp)
+{
+ struct sev_user_data_status out;
+ struct sev_data_status *data;
+ int ret;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error);
+ if (ret)
+ goto e_free;
+
+ out.api_major = data->api_major;
+ out.api_minor = data->api_minor;
+ out.state = data->state;
+ out.owner = data->owner;
+ out.config = data->config;
+ out.build = data->build;
+ out.guest_count = data->guest_count;
+ if (copy_to_user((void __user *)(uintptr_t) argp->data,
+ &out, sizeof(struct sev_user_data_status)))
+ ret = -EFAULT;
+
+e_free:
+ kfree(data);
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
void __user *argp = (void __user *)arg;
@@ -198,6 +228,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
break;
}
+ case SEV_PLATFORM_STATUS: {
+ ret = sev_ioctl_platform_status(&input);
+ break;
+ }
default:
ret = -EINVAL;
goto out;
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS " Brijesh Singh
2017-10-11 17:01 ` [Part2 PATCH v5.2 " Brijesh Singh
@ 2017-10-11 17:02 ` Borislav Petkov
2017-10-11 19:49 ` Brijesh Singh
1 sibling, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-11 17:02 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Fri, Oct 06, 2017 at 08:06:02PM -0500, Brijesh Singh wrote:
> The SEV_PLATFORM_STATUS command can be used by the platform owner to
> get the current status of the platform. The command is defined in
> SEV spec section 5.5.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> drivers/crypto/ccp/psp-dev.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
...
> @@ -198,6 +228,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> ret = sev_handle_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
> break;
> }
> + case SEV_PLATFORM_STATUS: {
> + ret = sev_ioctl_platform_status(&input);
> + break;
> + }
What's with the curly brackets around the case: statements?
Anyway, here are some more improvements:
* you can get rid of the struct copying into out and the bitfields by
doing something like this:
ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error);
if (ret)
goto e_free;
/* Clear out reserved fields: */
data->owner &= BIT(0);
data->config &= BIT(0);
I'm not sure those are the ones you need to clear but you get
the idea - you simply poke holes in the reserved fields before
copying to userspace. If you need a more sophisticated mask, use
GENMASK/GENMASK_ULL.
And then you don't need struct sev_user_data_status and
simply remove the bitfields too.
* Also, a function should have a verb in the name, thus
sev_ioctl_do_platform_status().
---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d668045956cb..1479db533da0 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -176,9 +176,8 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
return ret;
}
-static int sev_ioctl_platform_status(struct sev_issue_cmd *argp)
+static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
{
- struct sev_user_data_status out;
struct sev_data_status *data;
int ret;
@@ -186,19 +185,15 @@ static int sev_ioctl_platform_status(struct sev_issue_cmd *argp)
if (!data)
return -ENOMEM;
- ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error);
+ ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error);
if (ret)
goto e_free;
- out.api_major = data->api_major;
- out.api_minor = data->api_minor;
- out.state = data->state;
- out.owner = data->owner;
- out.config = data->config;
- out.build = data->build;
- out.guest_count = data->guest_count;
- if (copy_to_user((void __user *)(uintptr_t) argp->data,
- &out, sizeof(struct sev_user_data_status)))
+ /* Clear out reserved fields: */
+ data->owner &= BIT(0);
+ data->config &= BIT(0);
+
+ if (copy_to_user((void __user *)argp->data, data, sizeof(*data)))
ret = -EFAULT;
e_free:
@@ -226,10 +221,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, &input.error);
break;
- case SEV_PLATFORM_STATUS: {
- ret = sev_ioctl_platform_status(&input);
+ case SEV_PLATFORM_STATUS:
+ ret = sev_ioctl_do_platform_status(&input);
break;
- }
+
default:
ret = -EINVAL;
goto out;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 10b843cce75f..223942ba3e7e 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -144,11 +144,9 @@ struct sev_data_status {
u8 api_major; /* Out */
u8 api_minor; /* Out */
u8 state; /* Out */
- u8 owner : 1; /* Out */
- u8 reserved1 : 7;
- u32 config : 1; /* Out */
- u32 reserved2 : 23;
- u32 build : 8; /* Out */
+ u8 owner; /* Out */
+ u32 config; /* Out */
+ u32 build; /* Out */
u32 guest_count; /* Out */
} __packed;
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-11 17:02 ` [Part2 PATCH v5.1 " Borislav Petkov
@ 2017-10-11 19:49 ` Brijesh Singh
2017-10-11 20:04 ` Borislav Petkov
0 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-11 19:49 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/11/2017 12:02 PM, Borislav Petkov wrote:
...
>
> What's with the curly brackets around the case: statements?
>
I will remove the curly braces.
> Anyway, here are some more improvements:
>
> * you can get rid of the struct copying into out and the bitfields by
> doing something like this:
>
> ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, &argp->error);
> if (ret)
> goto e_free;
>
> /* Clear out reserved fields: */
> data->owner &= BIT(0);
> data->config &= BIT(0);
>
> I'm not sure those are the ones you need to clear but you get
> the idea - you simply poke holes in the reserved fields before
> copying to userspace. If you need a more sophisticated mask, use
> GENMASK/GENMASK_ULL.
>
If we decide to go with this approach then we need use GENMASK (see below).
...
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -144,11 +144,9 @@ struct sev_data_status {
Since the structure is shared with firmware hence I was trying to match
it with the spec.
> u8 api_major; /* Out */
> u8 api_minor; /* Out */
> u8 state; /* Out */
> - u8 owner : 1; /* Out */
> - u8 reserved1 : 7;
> - u32 config : 1; /* Out */
> - u32 reserved2 : 23;
> - u32 build : 8; /* Out */
> + u8 owner; /* Out */
This is OK for now. But in future if FW steals another bit from
reserved1 field to expose a new flag then 'owner' name will no longer be
valid. If you don't to use bit field then we have to use some generic
name instead of naming the field as 'owner'. Please note that its not
just userspace, KVM driver also uses the same fields and it may also
need to know which bit position to use.
> + u32 config; /* Out */
> + u32 build; /* Out */
This is a tricky one. The 32-bit are packed as:
BIT0 - config.es
BIT1-23: reserved
BIT24-31: build
Now, if we really don't want to use bit field then we have to declare
them as:
u8 config[3];
u8 build;
I would prefer to keep the structure as is. I am OK with changing the
userspace sev_user_data_status to match with FW's sev_data_status to
avoid the coying from FW structure to userspace structure.
> u32 guest_count; /* Out */
> } __packed;
>
>
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-11 19:49 ` Brijesh Singh
@ 2017-10-11 20:04 ` Borislav Petkov
2017-10-11 20:10 ` Borislav Petkov
2017-10-11 20:10 ` Brijesh Singh
0 siblings, 2 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-11 20:04 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Wed, Oct 11, 2017 at 02:49:55PM -0500, Brijesh Singh wrote:
> This is OK for now. But in future if FW steals another bit from reserved1
> field to expose a new flag then 'owner' name will no longer be valid. If you
> don't to use bit field then we have to use some generic name instead of
> naming the field as 'owner'. Please note that its not just userspace, KVM
> driver also uses the same fields and it may also need to know which bit
> position to use.
So what is this field called in the spec?
> This is a tricky one. The 32-bit are packed as:
>
> BIT0 - config.es
> BIT1-23: reserved
> BIT24-31: build
Is that what the firmware gives?
Then it is easy:
<firmware_field_name> &= GENMASK(23, 1);
and then userspace can pick apart bit 0 and bit24-31.
Just use one raw struct which the firmware gives you and the rest is
done by the sw. Like clearing reserved fields before copying to user.
You don't want to be updating that struct layout later: think of old
qemu with new kernel and all those different configurations.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-11 20:04 ` Borislav Petkov
@ 2017-10-11 20:10 ` Borislav Petkov
2017-10-11 20:10 ` Brijesh Singh
1 sibling, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-11 20:10 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Wed, Oct 11, 2017 at 10:04:44PM +0200, Borislav Petkov wrote:
> Then it is easy:
>
> <firmware_field_name> &= GENMASK(23, 1);
[1:23] range cleared, of course:
<firmware_field_name> &= ~GENMASK(23, 1);
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-11 20:04 ` Borislav Petkov
2017-10-11 20:10 ` Borislav Petkov
@ 2017-10-11 20:10 ` Brijesh Singh
2017-10-11 20:28 ` Borislav Petkov
1 sibling, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-11 20:10 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/11/2017 03:04 PM, Borislav Petkov wrote:
> On Wed, Oct 11, 2017 at 02:49:55PM -0500, Brijesh Singh wrote:
>> This is OK for now. But in future if FW steals another bit from reserved1
>> field to expose a new flag then 'owner' name will no longer be valid. If you
>> don't to use bit field then we have to use some generic name instead of
>> naming the field as 'owner'. Please note that its not just userspace, KVM
>> driver also uses the same fields and it may also need to know which bit
>> position to use.
>
> So what is this field called in the spec?
>
See Section 5.5.2 [1]
[1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
The bit0 is named as 'owner' and bits 7:1 is left blank (no name is given).
>> This is a tricky one. The 32-bit are packed as:
>>
>> BIT0 - config.es
>> BIT1-23: reserved
>> BIT24-31: build
>
> Is that what the firmware gives?
>
Yes
> Then it is easy:
>
> <firmware_field_name> &= GENMASK(23, 1);
>
> and then userspace can pick apart bit 0 and bit24-31.
>
The bit0 is named as config.es and bit1-23 is left blank and bit31-24 is
named as build.
The current 'struct sev_data_status' matches with the firmware names and
the bit fields. Only thing I did was the fields with no name is called
as "reservedX"
> Just use one raw struct which the firmware gives you and the rest is
> done by the sw. Like clearing reserved fields before copying to user.
>
> You don't want to be updating that struct layout later: think of old
> qemu with new kernel and all those different configurations.
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-11 20:10 ` Brijesh Singh
@ 2017-10-11 20:28 ` Borislav Petkov
2017-10-11 20:45 ` Brijesh Singh
0 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-11 20:28 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Wed, Oct 11, 2017 at 03:10:49PM -0500, Brijesh Singh wrote:
> The current 'struct sev_data_status' matches with the firmware names and the
> bit fields. Only thing I did was the fields with no name is called as
> "reservedX"
Ok, I see it. So what you actually wanna do is:
struct sev_data_status {
u8 api_major; /* Out */
u8 api_minor; /* Out */
u8 state; /* Out */
u8 flags; /* Out */
u32 config; /* Out */
u32 guest_count; /* Out */
} __packed;
as this is exactly what the firwmare gives you. Theoretically, you
could've also done:
struct sev_data_status {
u64 first_qword;
u32 second_dword;
};
but you have the fields mostly defined already and that would be too
confusing.
What I mean is, once you've gotten the command buffer, then you can pick
fields apart in software.
owner = status.flags & 1;
config_es = status.config & 1;
build = (status.config >> 24) & 0xff;
This way, if new fields get added, you don't have to change the struct
definitions - *especially* if they're visible to userspace - and users
of that struct can be extended to understand the new fields.
And before you copy the struct to userspace, you can simply clear out
the reserved fields as nothing should rely on them having a particular
value, because, well, they're reserved, doh.
Makes sense?
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-11 20:28 ` Borislav Petkov
@ 2017-10-11 20:45 ` Brijesh Singh
2017-10-11 20:53 ` Brijesh Singh
2017-10-11 20:54 ` Borislav Petkov
0 siblings, 2 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-11 20:45 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/11/2017 03:28 PM, Borislav Petkov wrote:
> On Wed, Oct 11, 2017 at 03:10:49PM -0500, Brijesh Singh wrote:
>> The current 'struct sev_data_status' matches with the firmware names and the
>> bit fields. Only thing I did was the fields with no name is called as
>> "reservedX"
>
> Ok, I see it. So what you actually wanna do is:
>
> struct sev_data_status {
> u8 api_major; /* Out */
> u8 api_minor; /* Out */
> u8 state; /* Out */
> u8 flags; /* Out */
> u32 config; /* Out */
> u32 guest_count; /* Out */
> } __packed;
>
OK, if userspace is going to pick bits apart then how about this:
struct sev_data_status {
u8 api_major; /* Out */
u8 api_minor; /* Out */
u8 state; /* Out */
u32 flags; /* Out */
u8 build; /* Out */
u32 guest_count; /* Out */
} __packed;
>
> Makes sense?
>
Please let me know if you are okay with my above structure.
-Brijesh
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-11 20:45 ` Brijesh Singh
@ 2017-10-11 20:53 ` Brijesh Singh
2017-10-11 20:54 ` Borislav Petkov
1 sibling, 0 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-11 20:53 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/11/2017 03:45 PM, Brijesh Singh wrote:
>
>
> On 10/11/2017 03:28 PM, Borislav Petkov wrote:
>> On Wed, Oct 11, 2017 at 03:10:49PM -0500, Brijesh Singh wrote:
>>> The current 'struct sev_data_status' matches with the firmware names
>>> and the
>>> bit fields. Only thing I did was the fields with no name is called as
>>> "reservedX"
>>
>> Ok, I see it. So what you actually wanna do is:
>>
>> struct sev_data_status {
>> u8 api_major; /* Out */
>> u8 api_minor; /* Out */
>> u8 state; /* Out */
>> u8 flags; /* Out */
>> u32 config; /* Out */
>> u32 guest_count; /* Out */
>> } __packed;
>>
>
> OK, if userspace is going to pick bits apart then how about this:
>
> struct sev_data_status {
> u8 api_major; /* Out */
> u8 api_minor; /* Out */
> u8 state; /* Out */
> u32 flags; /* Out */
> u8 build; /* Out */
> u32 guest_count; /* Out */
> } __packed;
>
BTW, I kept the build name because KVM driver prints some debug
information like this:
pr_info_once("SEV: api_major %d api_minor %d build %d\n",
status->api_major, status->api_minor, status->build);
Of course, I can modify it to access bit field if we decide to go with
your recommended structure - thanks
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command
2017-10-11 20:45 ` Brijesh Singh
2017-10-11 20:53 ` Brijesh Singh
@ 2017-10-11 20:54 ` Borislav Petkov
1 sibling, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-11 20:54 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Wed, Oct 11, 2017 at 03:45:00PM -0500, Brijesh Singh wrote:
> OK, if userspace is going to pick bits apart then how about this:
>
> struct sev_data_status {
> u8 api_major; /* Out */
> u8 api_minor; /* Out */
> u8 state; /* Out */
> u32 flags; /* Out */
> u8 build; /* Out */
I guess. Except the spec marks those as 31:24 and belonging to the dword
starting at offset 4 and you've made "build" a separate u8 and the dword
starts at offset 3. I mean, not a big deal, I think you can change the
spec for a change :-)
Because it looks like the 4 bytes starting at offset 3 really are meant
for miscellaneous bits. And then CONFIG.ES could've been bit 1 of the
byte at offset 3. Oh well...
> Please let me know if you are okay with my above structure.
But yeah, in the end of the day it probably doesn't matter a whole lot.
The spec just counts those in 32-bit quantities.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
` (2 preceding siblings ...)
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS " Brijesh Singh
@ 2017-10-07 1:06 ` Brijesh Singh
2017-10-12 18:28 ` Borislav Petkov
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN " Brijesh Singh
` (5 subsequent siblings)
9 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-07 1:06 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
The SEV_PEK_GEN command is used to generate a new Platform Endorsement
Key (PEK). The command is defined in SEV spec section 5.6.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
drivers/crypto/ccp/psp-dev.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d68303a06464..03d7bd03ad58 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -207,6 +207,70 @@ static int sev_ioctl_platform_status(struct sev_issue_cmd *argp)
return ret;
}
+static int sev_platform_get_state(int *state, int *error)
+{
+ struct sev_data_status *data;
+ int ret;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+ if (!ret)
+ *state = data->state;
+
+ kfree(data);
+ return ret;
+}
+
+static int sev_firmware_init(int *error)
+{
+ struct sev_data_init *data;
+ int rc;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ rc = sev_handle_cmd(SEV_CMD_INIT, data, error);
+
+ kfree(data);
+ return rc;
+}
+
+static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
+{
+ int do_shutdown = 0;
+ int ret, state;
+
+ /*
+ * PEK_GEN command can be issued only when firmware is in INIT state.
+ * If firmware is in UNINIT state then we transition it in INIT state
+ * and issue the command.
+ */
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ return ret;
+
+ if (state == SEV_STATE_WORKING) {
+ return -EBUSY;
+ } else if (state == SEV_STATE_UNINIT) {
+ ret = sev_firmware_init(&argp->error);
+ if (ret)
+ return ret;
+
+ do_shutdown = 1;
+ }
+
+ ret = sev_handle_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
+
+ if (do_shutdown)
+ sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
void __user *argp = (void __user *)arg;
@@ -232,6 +296,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_ioctl_platform_status(&input);
break;
}
+ case SEV_PEK_GEN: {
+ ret = sev_ioctl_pek_gen(&input);
+ break;
+ }
default:
ret = -EINVAL;
break;
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN " Brijesh Singh
@ 2017-10-12 18:28 ` Borislav Petkov
2017-10-12 20:11 ` Brijesh Singh
0 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 18:28 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Fri, Oct 06, 2017 at 08:06:03PM -0500, Brijesh Singh wrote:
> The SEV_PEK_GEN command is used to generate a new Platform Endorsement
> Key (PEK). The command is defined in SEV spec section 5.6.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> drivers/crypto/ccp/psp-dev.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
Just small fixups. Worth noting is this:
if (do_shutdown)
- sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+ ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
I think we want to return the result of the *last* command
executed, which, in the do_shutdown=1 case is SEV_CMD_SHUTDOWN, not
SEV_CMD_PEK_GEN.
---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d02f48e356e8..31045ea7e798 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -211,7 +211,7 @@ static int sev_platform_get_state(int *state, int *error)
if (!data)
return -ENOMEM;
- ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+ ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
if (!ret)
*state = data->state;
@@ -228,7 +228,7 @@ static int sev_firmware_init(int *error)
if (!data)
return -ENOMEM;
- rc = sev_handle_cmd(SEV_CMD_INIT, data, error);
+ rc = sev_do_cmd(SEV_CMD_INIT, data, error);
kfree(data);
return rc;
@@ -240,8 +240,8 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
int ret, state;
/*
- * PEK_GEN command can be issued only when firmware is in INIT state.
- * If firmware is in UNINIT state then we transition it in INIT state
+ * The PEK_GEN command can be issued only when the firmware is in INIT
+ * state. If it is in UNINIT state then we transition it in INIT state
* and issue the command.
*/
ret = sev_platform_get_state(&state, &argp->error);
@@ -258,10 +258,10 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
- ret = sev_handle_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
+ ret = sev_do_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
if (do_shutdown)
- sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+ ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
return ret;
}
@@ -291,10 +291,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_ioctl_do_platform_status(&input);
break;
- case SEV_PEK_GEN: {
+ case SEV_PEK_GEN:
ret = sev_ioctl_pek_gen(&input);
break;
- }
+
default:
ret = -EINVAL;
goto out;
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command
2017-10-12 18:28 ` Borislav Petkov
@ 2017-10-12 20:11 ` Brijesh Singh
2017-10-12 20:21 ` Borislav Petkov
0 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-12 20:11 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/12/17 1:28 PM, Borislav Petkov wrote:
> On Fri, Oct 06, 2017 at 08:06:03PM -0500, Brijesh Singh wrote:
>> The SEV_PEK_GEN command is used to generate a new Platform Endorsement
>> Key (PEK). The command is defined in SEV spec section 5.6.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: Gary Hook <gary.hook@amd.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: linux-crypto@vger.kernel.org
>> Cc: kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> drivers/crypto/ccp/psp-dev.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 68 insertions(+)
> Just small fixups. Worth noting is this:
>
> if (do_shutdown)
> - sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> + ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>
> I think we want to return the result of the *last* command
> executed, which, in the do_shutdown=1 case is SEV_CMD_SHUTDOWN, not
> SEV_CMD_PEK_GEN.
Lets consider this scenario
1- platform is in uninit state, we transition it to INIT
2- PEK_GEN command failed
3- since we have transitioned the platform in INIT state hence we must
call the shutdown otherwise we will leave the system in wrong state. The
shutdown command will most probably succeed and we will look the ret value
> ---
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index d02f48e356e8..31045ea7e798 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -211,7 +211,7 @@ static int sev_platform_get_state(int *state, int *error)
> if (!data)
> return -ENOMEM;
>
> - ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
> + ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
> if (!ret)
> *state = data->state;
>
> @@ -228,7 +228,7 @@ static int sev_firmware_init(int *error)
> if (!data)
> return -ENOMEM;
>
> - rc = sev_handle_cmd(SEV_CMD_INIT, data, error);
> + rc = sev_do_cmd(SEV_CMD_INIT, data, error);
>
> kfree(data);
> return rc;
> @@ -240,8 +240,8 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
> int ret, state;
>
> /*
> - * PEK_GEN command can be issued only when firmware is in INIT state.
> - * If firmware is in UNINIT state then we transition it in INIT state
> + * The PEK_GEN command can be issued only when the firmware is in INIT
> + * state. If it is in UNINIT state then we transition it in INIT state
> * and issue the command.
> */
> ret = sev_platform_get_state(&state, &argp->error);
> @@ -258,10 +258,10 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
> do_shutdown = 1;
> }
>
> - ret = sev_handle_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
> + ret = sev_do_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
>
> if (do_shutdown)
> - sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> + ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>
> return ret;
> }
> @@ -291,10 +291,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> ret = sev_ioctl_do_platform_status(&input);
> break;
>
> - case SEV_PEK_GEN: {
> + case SEV_PEK_GEN:
> ret = sev_ioctl_pek_gen(&input);
> break;
> - }
> +
> default:
> ret = -EINVAL;
> goto out;
>
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command
2017-10-12 20:11 ` Brijesh Singh
@ 2017-10-12 20:21 ` Borislav Petkov
2017-10-12 20:34 ` Brijesh Singh
0 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 20:21 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Thu, Oct 12, 2017 at 03:11:07PM -0500, Brijesh Singh wrote:
> Lets consider this scenario
> 1- platform is in uninit state, we transition it to INIT
> 2- PEK_GEN command failed
> 3- since we have transitioned the platform in INIT state hence we must
> call the shutdown otherwise we will leave the system in wrong state. The
> shutdown command will most probably succeed and we will look the ret value
Sure but what do you do if the main command, i.e., PEK_GEN succeeds but
the shutdown command fails?
You probably should carve out the whole shutdown order in separate
functions. I mean, the sequences do repeat in a couple of functions so
you could do:
ioctl:
case <CMD>:
init_platform()
do_main_cmd()
shutdown_platform()
break;
and this way you have everything nicely separated and retvals properly
tracked...
Hmmm?
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command
2017-10-12 20:21 ` Borislav Petkov
@ 2017-10-12 20:34 ` Brijesh Singh
0 siblings, 0 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-12 20:34 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/12/17 3:21 PM, Borislav Petkov wrote:
> On Thu, Oct 12, 2017 at 03:11:07PM -0500, Brijesh Singh wrote:
>> Lets consider this scenario
>> 1- platform is in uninit state, we transition it to INIT
>> 2- PEK_GEN command failed
>> 3- since we have transitioned the platform in INIT state hence we must
>> call the shutdown otherwise we will leave the system in wrong state. The
>> shutdown command will most probably succeed and we will look the ret value
> Sure but what do you do if the main command, i.e., PEK_GEN succeeds but
> the shutdown command fails?
>
> You probably should carve out the whole shutdown order in separate
> functions. I mean, the sequences do repeat in a couple of functions so
> you could do:
>
> ioctl:
>
> case <CMD>:
>
> init_platform()
> do_main_cmd()
> shutdown_platform()
> break;
>
> and this way you have everything nicely separated and retvals properly
> tracked...
>
> Hmmm?
Some commands are allowed in INIT and WORKING, some in UINIT only, some
WORKING, and others in all the state. We need to follow the platform
state machine. I will see what I can do. thanks
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
` (3 preceding siblings ...)
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN " Brijesh Singh
@ 2017-10-07 1:06 ` Brijesh Singh
2017-10-12 18:48 ` Borislav Petkov
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR " Brijesh Singh
` (4 subsequent siblings)
9 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-07 1:06 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
The SEV_PDH_GEN command is used to re-generate the Platform
Diffie-Hellman (PDH) key. The command is defined in SEV spec section
5.9.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
drivers/crypto/ccp/psp-dev.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 03d7bd03ad58..28efb7a9245a 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -271,6 +271,34 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
return ret;
}
+static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp)
+{
+ int ret, state, do_shutdown = 0;
+
+ /*
+ * PDH_GEN command can be issued when platform is in INIT or WORKING
+ * state. If we are in UNINIT state then transition in INIT state
+ * before issuing the command.
+ */
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ return ret;
+
+ if (state == SEV_STATE_UNINIT) {
+ ret = sev_firmware_init(&argp->error);
+ if (ret)
+ return ret;
+ do_shutdown = 1;
+ }
+
+ ret = sev_handle_cmd(SEV_CMD_PDH_GEN, 0, &argp->error);
+
+ if (do_shutdown)
+ sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
void __user *argp = (void __user *)arg;
@@ -300,6 +328,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_ioctl_pek_gen(&input);
break;
}
+ case SEV_PDH_GEN: {
+ ret = sev_ioctl_pdh_gen(&input);
+ break;
+ }
default:
ret = -EINVAL;
break;
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN " Brijesh Singh
@ 2017-10-12 18:48 ` Borislav Petkov
2017-10-12 20:21 ` Brijesh Singh
0 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 18:48 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Fri, Oct 06, 2017 at 08:06:04PM -0500, Brijesh Singh wrote:
> The SEV_PDH_GEN command is used to re-generate the Platform
> Diffie-Hellman (PDH) key. The command is defined in SEV spec section
> 5.9.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> drivers/crypto/ccp/psp-dev.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 03d7bd03ad58..28efb7a9245a 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -271,6 +271,34 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
> return ret;
> }
>
> +static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp)
> +{
> + int ret, state, do_shutdown = 0;
> +
> + /*
> + * PDH_GEN command can be issued when platform is in INIT or WORKING
> + * state. If we are in UNINIT state then transition in INIT state
> + * before issuing the command.
> + */
> + ret = sev_platform_get_state(&state, &argp->error);
> + if (ret)
> + return ret;
> +
Why isn't this function doing:
if (state == SEV_STATE_WORKING) {
return -EBUSY;
like the PEK_GEN one?
Because if so, you can convert it and the PEK_GEN one into a single
function doing the work and wrappers handing in the command to avoid the
code duplication.
> + if (state == SEV_STATE_UNINIT) {
> + ret = sev_firmware_init(&argp->error);
> + if (ret)
> + return ret;
> + do_shutdown = 1;
> + }
> +
> + ret = sev_handle_cmd(SEV_CMD_PDH_GEN, 0, &argp->error);
> +
> + if (do_shutdown)
> + sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> +
> + return ret;
> +}
> +
> static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> {
> void __user *argp = (void __user *)arg;
> @@ -300,6 +328,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> ret = sev_ioctl_pek_gen(&input);
> break;
> }
> + case SEV_PDH_GEN: {
> + ret = sev_ioctl_pdh_gen(&input);
> + break;
> + }
And those curly braces can go, as before.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command
2017-10-12 18:48 ` Borislav Petkov
@ 2017-10-12 20:21 ` Brijesh Singh
2017-10-12 20:23 ` Borislav Petkov
0 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-12 20:21 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/12/17 1:48 PM, Borislav Petkov wrote:
...
> On Fri, Oct 06, 2017 at 08:06:04PM -0500, Brijesh Singh wrote:
>> The SEV_PDH_GEN command is used to re-generate the Platform
>> Diffie-Hellman (PDH) key. The command is defined in SEV spec section
>> 5.9.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: Gary Hook <gary.hook@amd.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: linux-crypto@vger.kernel.org
>> Cc: kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> drivers/crypto/ccp/psp-dev.c | 32 ++++++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>> index 03d7bd03ad58..28efb7a9245a 100644
>> --- a/drivers/crypto/ccp/psp-dev.c
>> +++ b/drivers/crypto/ccp/psp-dev.c
>> @@ -271,6 +271,34 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
>> return ret;
>> }
>>
>> +static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp)
>> +{
>> + int ret, state, do_shutdown = 0;
>> +
>> + /*
>> + * PDH_GEN command can be issued when platform is in INIT or WORKING
>> + * state. If we are in UNINIT state then transition in INIT state
>> + * before issuing the command.
>> + */
>> + ret = sev_platform_get_state(&state, &argp->error);
>> + if (ret)
>> + return ret;
>> +
> Why isn't this function doing:
>
> if (state == SEV_STATE_WORKING) {
> return -EBUSY;
>
> like the PEK_GEN one?
We need to follow the platform state machine logic defined in SEV spec
section 5.1.2. The PEK_GEN can not be issued when platform is in WORKING
state because the command actually re-generate the identity of the
platform itself (in other words re-generate the Platform Endorsement
Key). Whereas, the PDH_GEN command is used for re-generating Platform
Diffie-Hellman Key which can be changed while the guest is running.
> Because if so, you can convert it and the PEK_GEN one into a single
> function doing the work and wrappers handing in the command to avoid the
> code duplication.
>
>> + if (state == SEV_STATE_UNINIT) {
>> + ret = sev_firmware_init(&argp->error);
>> + if (ret)
>> + return ret;
>> + do_shutdown = 1;
>> + }
>> +
>> + ret = sev_handle_cmd(SEV_CMD_PDH_GEN, 0, &argp->error);
>> +
>> + if (do_shutdown)
>> + sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>> +
>> + return ret;
>> +}
>> +
>> static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>> {
>> void __user *argp = (void __user *)arg;
>> @@ -300,6 +328,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>> ret = sev_ioctl_pek_gen(&input);
>> break;
>> }
>> + case SEV_PDH_GEN: {
>> + ret = sev_ioctl_pdh_gen(&input);
>> + break;
>> + }
> And those curly braces can go, as before.
>
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command
2017-10-12 20:21 ` Brijesh Singh
@ 2017-10-12 20:23 ` Borislav Petkov
0 siblings, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 20:23 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Thu, Oct 12, 2017 at 03:21:04PM -0500, Brijesh Singh wrote:
> We need to follow the platform state machine logic defined in SEV spec
> section 5.1.2. The PEK_GEN can not be issued when platform is in WORKING
> state because the command actually re-generate the identity of the
> platform itself (in other words re-generate the Platform Endorsement
> Key). Whereas, the PDH_GEN command is used for re-generating Platform
> Diffie-Hellman Key which can be changed while the guest is running.
I see.
So the proposition to carve out and split the platform *init commands
might come in handy here too...
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
` (4 preceding siblings ...)
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN " Brijesh Singh
@ 2017-10-07 1:06 ` Brijesh Singh
2017-10-12 19:53 ` Borislav Petkov
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.8/31] crypto: ccp: Implement SEV_PEK_CERT_IMPORT " Brijesh Singh
` (3 subsequent siblings)
9 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-07 1:06 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
The SEV_PEK_CSR command can be used to generate a PEK certificate
signing request. The command is defined in SEV spec section 5.7.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
drivers/crypto/ccp/psp-dev.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 85 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 28efb7a9245a..8038ca7aef03 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -299,6 +299,87 @@ static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp)
return ret;
}
+static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
+{
+ struct sev_user_data_pek_csr input;
+ struct sev_data_pek_csr *data;
+ int do_shutdown = 0;
+ int ret, state;
+ void *blob;
+
+ if (copy_from_user(&input, (void __user *)(uintptr_t)argp->data,
+ sizeof(struct sev_user_data_pek_csr)))
+ return -EFAULT;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ /* allocate a temporary physical contigous buffer to store the CSR blob */
+ blob = NULL;
+ if (input.address) {
+ if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
+ input.length > SEV_FW_BLOB_MAX_SIZE) {
+ ret = -EFAULT;
+ goto e_free;
+ }
+
+ blob = kmalloc(input.length, GFP_KERNEL);
+ if (!blob) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ data->address = __psp_pa(blob);
+ data->len = input.length;
+ }
+
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ goto e_free_blob;
+
+ /*
+ * PEK_CERT command can be issued only when we are in INIT state.
+ * if current state is WORKING then reject it, if state is UNINIT
+ * then transition the platform to INIT state before issuing the
+ * command.
+ */
+ if (state == SEV_STATE_WORKING) {
+ ret = -EBUSY;
+ goto e_free_blob;
+ } else if (state == SEV_STATE_UNINIT) {
+ ret = sev_firmware_init(&argp->error);
+ if (ret)
+ goto e_free_blob;
+ do_shutdown = 1;
+ }
+
+ ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
+
+ input.length = data->len;
+
+ /* copy blob to userspace */
+ if (blob &&
+ copy_to_user((void __user *)(uintptr_t)input.address,
+ blob, input.length)) {
+ ret = -EFAULT;
+ goto e_shutdown;
+ }
+
+ if (copy_to_user((void __user *)(uintptr_t)argp->data, &input,
+ sizeof(struct sev_user_data_pek_csr)))
+ ret = -EFAULT;
+
+e_shutdown:
+ if (do_shutdown)
+ sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+e_free_blob:
+ kfree(blob);
+e_free:
+ kfree(data);
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
void __user *argp = (void __user *)arg;
@@ -332,6 +413,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_ioctl_pdh_gen(&input);
break;
}
+ case SEV_PEK_CSR: {
+ ret = sev_ioctl_pek_csr(&input);
+ break;
+ }
default:
ret = -EINVAL;
break;
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR " Brijesh Singh
@ 2017-10-12 19:53 ` Borislav Petkov
2017-10-13 2:24 ` Brijesh Singh
0 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 19:53 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Fri, Oct 06, 2017 at 08:06:05PM -0500, Brijesh Singh wrote:
> The SEV_PEK_CSR command can be used to generate a PEK certificate
> signing request. The command is defined in SEV spec section 5.7.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> drivers/crypto/ccp/psp-dev.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 85 insertions(+)
Ok, a couple of things here:
* Move the checks first and the allocations second so that you allocate
memory only after all checks have been passed and you don't allocate
pointlessly.
* That:
if (state == SEV_STATE_WORKING) {
ret = -EBUSY;
goto e_free_blob;
} else if (state == SEV_STATE_UNINIT) {
ret = sev_firmware_init(&argp->error);
if (ret)
goto e_free_blob;
do_shutdown = 1;
}
is a repeating pattern. Perhaps it should be called
sev_firmware_reinit() and called by other functions.
* The rest is simplifications and streamlining.
---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e3ee68afd068..d41f5448a25b 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
int ret, state;
void *blob;
- if (copy_from_user(&input, (void __user *)(uintptr_t)argp->data,
- sizeof(struct sev_user_data_pek_csr)))
+ if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
+ return -EFAULT;
+
+ if (!input.address)
+ return -EINVAL;
+
+ /* allocate a physically contiguous buffer to store the CSR blob */
+ if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
+ input.length > SEV_FW_BLOB_MAX_SIZE)
return -EFAULT;
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
- /* allocate a temporary physical contigous buffer to store the CSR blob */
- blob = NULL;
- if (input.address) {
- if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
- input.length > SEV_FW_BLOB_MAX_SIZE) {
- ret = -EFAULT;
- goto e_free;
- }
-
- blob = kmalloc(input.length, GFP_KERNEL);
- if (!blob) {
- ret = -ENOMEM;
- goto e_free;
- }
-
- data->address = __psp_pa(blob);
- data->len = input.length;
+ blob = kmalloc(input.length, GFP_KERNEL);
+ if (!blob) {
+ ret = -ENOMEM;
+ goto e_free;
}
+ data->address = __psp_pa(blob);
+ data->len = input.length;
+
ret = sev_platform_get_state(&state, &argp->error);
if (ret)
goto e_free_blob;
@@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
- ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
+ ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
input.length = data->len;
/* copy blob to userspace */
- if (blob &&
- copy_to_user((void __user *)(uintptr_t)input.address,
- blob, input.length)) {
+ if (copy_to_user((void __user *)input.address, blob, input.length)) {
ret = -EFAULT;
goto e_shutdown;
}
- if (copy_to_user((void __user *)(uintptr_t)argp->data, &input,
- sizeof(struct sev_user_data_pek_csr)))
+ if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
ret = -EFAULT;
e_shutdown:
if (do_shutdown)
- sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+ ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+
e_free_blob:
kfree(blob);
e_free:
@@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_ioctl_pdh_gen(&input);
break;
- case SEV_PEK_CSR: {
+ case SEV_PEK_CSR:
ret = sev_ioctl_pek_csr(&input);
break;
- }
+
default:
ret = -EINVAL;
goto out;
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command
2017-10-12 19:53 ` Borislav Petkov
@ 2017-10-13 2:24 ` Brijesh Singh
2017-10-13 4:13 ` Brijesh Singh
2017-10-13 9:14 ` Borislav Petkov
0 siblings, 2 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-13 2:24 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/12/17 2:53 PM, Borislav Petkov wrote:
...
> Ok, a couple of things here:
>
> * Move the checks first and the allocations second so that you allocate
> memory only after all checks have been passed and you don't allocate
> pointlessly.
I assume you mean performing the SEV state check before allocating the
memory for the CSR blob, right ? In my patches, I typically perform all
the SW specific checks and allocation before invoking the HW routines.
Handling the PSP commands will take longer compare to kmalloc() or
access_ok() etc. If its not a big deal then I would prefer to keep that
way.
>
> * That:
>
> if (state == SEV_STATE_WORKING) {
> ret = -EBUSY;
> goto e_free_blob;
> } else if (state == SEV_STATE_UNINIT) {
> ret = sev_firmware_init(&argp->error);
> if (ret)
> goto e_free_blob;
> do_shutdown = 1;
> }
>
> is a repeating pattern. Perhaps it should be called
> sev_firmware_reinit() and called by other functions.
> * The rest is simplifications and streamlining.
>
> ---
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index e3ee68afd068..d41f5448a25b 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
> int ret, state;
> void *blob;
>
> - if (copy_from_user(&input, (void __user *)(uintptr_t)argp->data,
> - sizeof(struct sev_user_data_pek_csr)))
> + if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> + return -EFAULT;
> +
> + if (!input.address)
> + return -EINVAL;
> +
> + /* allocate a physically contiguous buffer to store the CSR blob */
> + if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
> + input.length > SEV_FW_BLOB_MAX_SIZE)
> return -EFAULT;
>
> data = kzalloc(sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
>
> - /* allocate a temporary physical contigous buffer to store the CSR blob */
> - blob = NULL;
> - if (input.address) {
> - if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
> - input.length > SEV_FW_BLOB_MAX_SIZE) {
> - ret = -EFAULT;
> - goto e_free;
> - }
> -
> - blob = kmalloc(input.length, GFP_KERNEL);
> - if (!blob) {
> - ret = -ENOMEM;
> - goto e_free;
> - }
> -
> - data->address = __psp_pa(blob);
> - data->len = input.length;
> + blob = kmalloc(input.length, GFP_KERNEL);
> + if (!blob) {
> + ret = -ENOMEM;
> + goto e_free;
> }
>
> + data->address = __psp_pa(blob);
> + data->len = input.length;
> +
> ret = sev_platform_get_state(&state, &argp->error);
> if (ret)
> goto e_free_blob;
> @@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
> do_shutdown = 1;
> }
>
> - ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
> + ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
>
> input.length = data->len;
>
> /* copy blob to userspace */
> - if (blob &&
> - copy_to_user((void __user *)(uintptr_t)input.address,
> - blob, input.length)) {
> + if (copy_to_user((void __user *)input.address, blob, input.length)) {
> ret = -EFAULT;
> goto e_shutdown;
> }
>
> - if (copy_to_user((void __user *)(uintptr_t)argp->data, &input,
> - sizeof(struct sev_user_data_pek_csr)))
> + if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
> ret = -EFAULT;
>
> e_shutdown:
> if (do_shutdown)
> - sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> + ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> +
> e_free_blob:
> kfree(blob);
> e_free:
> @@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> ret = sev_ioctl_pdh_gen(&input);
> break;
>
> - case SEV_PEK_CSR: {
> + case SEV_PEK_CSR:
> ret = sev_ioctl_pek_csr(&input);
> break;
> - }
> +
> default:
> ret = -EINVAL;
> goto out;
>
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command
2017-10-13 2:24 ` Brijesh Singh
@ 2017-10-13 4:13 ` Brijesh Singh
2017-10-13 10:20 ` Borislav Petkov
2017-10-13 9:14 ` Borislav Petkov
1 sibling, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-13 4:13 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/12/17 9:24 PM, Brijesh Singh wrote:
>
> On 10/12/17 2:53 PM, Borislav Petkov wrote:
> ...
>
>> Ok, a couple of things here:
>>
>> * Move the checks first and the allocations second so that you allocate
>> memory only after all checks have been passed and you don't allocate
>> pointlessly.
>
> I assume you mean performing the SEV state check before allocating the
> memory for the CSR blob, right ? In my patches, I typically perform all
> the SW specific checks and allocation before invoking the HW routines.
> Handling the PSP commands will take longer compare to kmalloc() or
> access_ok() etc. If its not a big deal then I would prefer to keep that
> way.
>
>
>> * That:
>>
>> if (state == SEV_STATE_WORKING) {
>> ret = -EBUSY;
>> goto e_free_blob;
>> } else if (state == SEV_STATE_UNINIT) {
>> ret = sev_firmware_init(&argp->error);
>> if (ret)
>> goto e_free_blob;
>> do_shutdown = 1;
>> }
>>
>> is a repeating pattern. Perhaps it should be called
>> sev_firmware_reinit() and called by other functions.
>
>> * The rest is simplifications and streamlining.
>>
>> ---
>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>> index e3ee68afd068..d41f5448a25b 100644
>> --- a/drivers/crypto/ccp/psp-dev.c
>> +++ b/drivers/crypto/ccp/psp-dev.c
>> @@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
>> int ret, state;
>> void *blob;
>>
>> - if (copy_from_user(&input, (void __user *)(uintptr_t)argp->data,
>> - sizeof(struct sev_user_data_pek_csr)))
>> + if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>> + return -EFAULT;
>> +
>> + if (!input.address)
>> + return -EINVAL;
>> +
As per the spec, its perfectly legal to pass input.address = 0x0 and
input.length = 0x0. If userspace wants to query CSR length then it will
fill all the fields with. In response, FW will return error
(LENGTH_TO_SMALL) and input.length will be filled with the expected
length. Several command work very similar (e.g PEK_CSR,
PEK_CERT_EXPORT). A typical usage from userspace will be:
- query the length of the blob (call command with all fields set to zero)
- SEV FW will response with expected length
- userspace allocate the blob and retries the command.
>> + /* allocate a physically contiguous buffer to store the CSR blob */
>> + if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
>> + input.length > SEV_FW_BLOB_MAX_SIZE)
>> return -EFAULT;
>>
>> data = kzalloc(sizeof(*data), GFP_KERNEL);
>> if (!data)
>> return -ENOMEM;
>>
>> - /* allocate a temporary physical contigous buffer to store the CSR blob */
>> - blob = NULL;
>> - if (input.address) {
>> - if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
>> - input.length > SEV_FW_BLOB_MAX_SIZE) {
>> - ret = -EFAULT;
>> - goto e_free;
>> - }
>> -
>> - blob = kmalloc(input.length, GFP_KERNEL);
>> - if (!blob) {
>> - ret = -ENOMEM;
>> - goto e_free;
>> - }
>> -
>> - data->address = __psp_pa(blob);
>> - data->len = input.length;
>> + blob = kmalloc(input.length, GFP_KERNEL);
>> + if (!blob) {
>> + ret = -ENOMEM;
>> + goto e_free;
>> }
>>
>> + data->address = __psp_pa(blob);
>> + data->len = input.length;
>> +
>> ret = sev_platform_get_state(&state, &argp->error);
>> if (ret)
>> goto e_free_blob;
>> @@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
>> do_shutdown = 1;
>> }
>>
>> - ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
>> + ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
>>
>> input.length = data->len;
>>
>> /* copy blob to userspace */
>> - if (blob &&
>> - copy_to_user((void __user *)(uintptr_t)input.address,
>> - blob, input.length)) {
>> + if (copy_to_user((void __user *)input.address, blob, input.length)) {
>> ret = -EFAULT;
>> goto e_shutdown;
>> }
>>
>> - if (copy_to_user((void __user *)(uintptr_t)argp->data, &input,
>> - sizeof(struct sev_user_data_pek_csr)))
>> + if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
>> ret = -EFAULT;
>>
>> e_shutdown:
>> if (do_shutdown)
>> - sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>> + ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>> +
>> e_free_blob:
>> kfree(blob);
>> e_free:
>> @@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>> ret = sev_ioctl_pdh_gen(&input);
>> break;
>>
>> - case SEV_PEK_CSR: {
>> + case SEV_PEK_CSR:
>> ret = sev_ioctl_pek_csr(&input);
>> break;
>> - }
>> +
>> default:
>> ret = -EINVAL;
>> goto out;
>>
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command
2017-10-13 4:13 ` Brijesh Singh
@ 2017-10-13 10:20 ` Borislav Petkov
0 siblings, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-13 10:20 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Thu, Oct 12, 2017 at 11:13:44PM -0500, Brijesh Singh wrote:
> As per the spec, its perfectly legal to pass input.address = 0x0 and
> input.length = 0x0. If userspace wants to query CSR length then it will
> fill all the fields with. In response, FW will return error
> (LENGTH_TO_SMALL) and input.length will be filled with the expected
> length. Several command work very similar (e.g PEK_CSR,
> PEK_CERT_EXPORT). A typical usage from userspace will be:
>
> - query the length of the blob (call command with all fields set to zero)
> - SEV FW will response with expected length
> - userspace allocate the blob and retries the command.
Ok, let's make that a clearer and more precise by explicitly checking
the query case:
+ /* Userspace wants to query CSR length */
+ if (!input.address && !input.length)
+ goto cmd;
and commenting why we're doing this.
The rest is cleanups.
---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e3ee68afd068..e10f507f9a60 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -299,36 +299,37 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
struct sev_user_data_pek_csr input;
struct sev_data_pek_csr *data;
int do_shutdown = 0;
+ void *blob = NULL;
int ret, state;
- void *blob;
- if (copy_from_user(&input, (void __user *)(uintptr_t)argp->data,
- sizeof(struct sev_user_data_pek_csr)))
+ if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
- /* allocate a temporary physical contigous buffer to store the CSR blob */
- blob = NULL;
- if (input.address) {
- if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
- input.length > SEV_FW_BLOB_MAX_SIZE) {
- ret = -EFAULT;
- goto e_free;
- }
+ /* Userspace wants to query CSR length */
+ if (!input.address && !input.length)
+ goto cmd;
- blob = kmalloc(input.length, GFP_KERNEL);
- if (!blob) {
- ret = -ENOMEM;
- goto e_free;
- }
+ /* allocate a physically contiguous buffer to store the CSR blob */
+ if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
+ input.length > SEV_FW_BLOB_MAX_SIZE) {
+ ret = -EFAULT;
+ goto e_free;
+ }
- data->address = __psp_pa(blob);
- data->len = input.length;
+ blob = kmalloc(input.length, GFP_KERNEL);
+ if (!blob) {
+ ret = -ENOMEM;
+ goto e_free;
}
+ data->address = __psp_pa(blob);
+ data->len = input.length;
+
+cmd:
ret = sev_platform_get_state(&state, &argp->error);
if (ret)
goto e_free_blob;
@@ -349,25 +350,26 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
- ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
+ ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
+ /*
+ * If we query the CSR length, FW responded with the expected length.
+ */
input.length = data->len;
- /* copy blob to userspace */
- if (blob &&
- copy_to_user((void __user *)(uintptr_t)input.address,
- blob, input.length)) {
- ret = -EFAULT;
- goto e_shutdown;
+ if (blob) {
+ if (copy_to_user((void __user *)input.address, blob, input.length)) {
+ ret = -EFAULT;
+ goto e_shutdown;
+ }
}
- if (copy_to_user((void __user *)(uintptr_t)argp->data, &input,
- sizeof(struct sev_user_data_pek_csr)))
+ if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
ret = -EFAULT;
e_shutdown:
if (do_shutdown)
- sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+ sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
e_free_blob:
kfree(blob);
e_free:
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread
* Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command
2017-10-13 2:24 ` Brijesh Singh
2017-10-13 4:13 ` Brijesh Singh
@ 2017-10-13 9:14 ` Borislav Petkov
1 sibling, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-13 9:14 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Thu, Oct 12, 2017 at 09:24:01PM -0500, Brijesh Singh wrote:
> I assume you mean performing the SEV state check before allocating the
> memory for the CSR blob, right ?
I mean, do those first:
if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
if (!input.address)
return -EINVAL;
/* allocate a physically contiguous buffer to store the CSR blob */
if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
input.length > SEV_FW_BLOB_MAX_SIZE)
return -EFAULT;
Because if you allocate the memory first and some of those checks fail,
you allocate in vain to free it immediately after. And you can save
yourself all that.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Part2 PATCH v5.1 12.8/31] crypto: ccp: Implement SEV_PEK_CERT_IMPORT ioctl command
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
` (5 preceding siblings ...)
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR " Brijesh Singh
@ 2017-10-07 1:06 ` Brijesh Singh
2017-10-13 14:53 ` Borislav Petkov
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.9/31] crypto: ccp: Implement SEV_PDH_CERT_EXPORT " Brijesh Singh
` (2 subsequent siblings)
9 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-07 1:06 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
The SEV_PEK_CERT_IMPORT command can be used to import the signed PEK
certificate. The command is defined in SEV spec section 5.8.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
drivers/crypto/ccp/psp-dev.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 8038ca7aef03..861c44bf2910 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -380,6 +380,99 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
return ret;
}
+static void *copy_user_blob(u64 __user uaddr, u32 len)
+{
+ void *data;
+
+ if (!uaddr || !len)
+ return ERR_PTR(-EINVAL);
+
+ /* verify that blob length does not exceed our limit */
+ if (len > SEV_FW_BLOB_MAX_SIZE)
+ return ERR_PTR(-EINVAL);
+
+ data = kmalloc(len, GFP_KERNEL);
+ if (!data)
+ return ERR_PTR(-ENOMEM);
+
+ if (copy_from_user(data, (void __user *)(uintptr_t)uaddr, len))
+ goto e_free;
+
+ return data;
+
+e_free:
+ kfree(data);
+ return ERR_PTR(-EFAULT);
+}
+
+static int sev_ioctl_pek_cert_import(struct sev_issue_cmd *argp)
+{
+ struct sev_user_data_pek_cert_import input;
+ struct sev_data_pek_cert_import *data;
+ int ret, state, do_shutdown = 0;
+ void *pek_blob, *oca_blob;
+
+ if (copy_from_user(&input, (void __user *)(uintptr_t) argp->data,
+ sizeof(struct sev_user_data_pek_cert_import)))
+ return -EFAULT;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ /* copy PEK certificate blobs from userspace */
+ pek_blob = copy_user_blob(input.pek_cert_address, input.pek_cert_len);
+ if (IS_ERR(pek_blob)) {
+ ret = PTR_ERR(pek_blob);
+ goto e_free;
+ }
+
+ data->pek_cert_address = __psp_pa(pek_blob);
+ data->pek_cert_len = input.pek_cert_len;
+
+ /* copy PEK certificate blobs from userspace */
+ oca_blob = copy_user_blob(input.oca_cert_address, input.oca_cert_len);
+ if (IS_ERR(oca_blob)) {
+ ret = PTR_ERR(oca_blob);
+ goto e_free_pek;
+ }
+
+ data->oca_cert_address = __psp_pa(oca_blob);
+ data->oca_cert_len = input.oca_cert_len;
+
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ goto e_free_oca;
+
+ /*
+ * PEK_CERT_IMPORT command can be issued only when platform is in INIT
+ * state. If we are in UNINIT state then transition in INIT state
+ * before issuing the command.
+ */
+ if (state == SEV_STATE_WORKING) {
+ ret = -EBUSY;
+ goto e_free_oca;
+ } else if (state == SEV_STATE_UNINIT) {
+ ret = sev_firmware_init(&argp->error);
+ if (ret)
+ goto e_free_oca;
+ do_shutdown = 1;
+ }
+
+ ret = sev_handle_cmd(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
+
+ if (do_shutdown)
+ sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+
+e_free_oca:
+ kfree(oca_blob);
+e_free_pek:
+ kfree(pek_blob);
+e_free:
+ kfree(data);
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
void __user *argp = (void __user *)arg;
@@ -417,6 +510,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_ioctl_pek_csr(&input);
break;
}
+ case SEV_PEK_CERT_IMPORT: {
+ ret = sev_ioctl_pek_cert_import(&input);
+ break;
+ }
default:
ret = -EINVAL;
break;
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.8/31] crypto: ccp: Implement SEV_PEK_CERT_IMPORT ioctl command
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.8/31] crypto: ccp: Implement SEV_PEK_CERT_IMPORT " Brijesh Singh
@ 2017-10-13 14:53 ` Borislav Petkov
2017-10-13 16:09 ` Brijesh Singh
0 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-13 14:53 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Fri, Oct 06, 2017 at 08:06:06PM -0500, Brijesh Singh wrote:
> The SEV_PEK_CERT_IMPORT command can be used to import the signed PEK
> certificate. The command is defined in SEV spec section 5.8.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> drivers/crypto/ccp/psp-dev.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
Just minor cleanups, otherwise looks ok.
---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 25a437c8d532..e61758af288f 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -392,7 +392,7 @@ static void *copy_user_blob(u64 __user uaddr, u32 len)
if (!data)
return ERR_PTR(-ENOMEM);
- if (copy_from_user(data, (void __user *)(uintptr_t)uaddr, len))
+ if (copy_from_user(data, (void __user *)uaddr, len))
goto e_free;
return data;
@@ -409,8 +409,7 @@ static int sev_ioctl_pek_cert_import(struct sev_issue_cmd *argp)
int ret, state, do_shutdown = 0;
void *pek_blob, *oca_blob;
- if (copy_from_user(&input, (void __user *)(uintptr_t) argp->data,
- sizeof(struct sev_user_data_pek_cert_import)))
+ if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
data = kzalloc(sizeof(*data), GFP_KERNEL);
@@ -456,10 +455,10 @@ static int sev_ioctl_pek_cert_import(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
- ret = sev_handle_cmd(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
+ ret = sev_do_cmd(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
if (do_shutdown)
- sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+ sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
e_free_oca:
kfree(oca_blob);
@@ -503,14 +502,14 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_ioctl_pdh_gen(&input);
break;
- case SEV_PEK_CSR: {
+ case SEV_PEK_CSR:
ret = sev_ioctl_pek_csr(&input);
break;
- }
- case SEV_PEK_CERT_IMPORT: {
+
+ case SEV_PEK_CERT_IMPORT:
ret = sev_ioctl_pek_cert_import(&input);
break;
- }
+
default:
ret = -EINVAL;
goto out;
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.8/31] crypto: ccp: Implement SEV_PEK_CERT_IMPORT ioctl command
2017-10-13 14:53 ` Borislav Petkov
@ 2017-10-13 16:09 ` Brijesh Singh
0 siblings, 0 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-13 16:09 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/13/2017 09:53 AM, Borislav Petkov wrote:
...
>
> - if (copy_from_user(data, (void __user *)(uintptr_t)uaddr, len))
> + if (copy_from_user(data, (void __user *)uaddr, len))
> goto e_free;
IIRC, typecast was needed for i386 build, but now we have depends on
X86_64 hence I will go ahead and remove the typecast from all other places.
>
> return data;
> @@ -409,8 +409,7 @@ static int sev_ioctl_pek_cert_import(struct sev_issue_cmd *argp)
> int ret, state, do_shutdown = 0;
> void *pek_blob, *oca_blob;
>
> - if (copy_from_user(&input, (void __user *)(uintptr_t) argp->data,
> - sizeof(struct sev_user_data_pek_cert_import)))
> + if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> return -EFAULT;
>
> data = kzalloc(sizeof(*data), GFP_KERNEL);
> @@ -456,10 +455,10 @@ static int sev_ioctl_pek_cert_import(struct sev_issue_cmd *argp)
> do_shutdown = 1;
> }
>
> - ret = sev_handle_cmd(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
> + ret = sev_do_cmd(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
>
> if (do_shutdown)
> - sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> + sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>
> e_free_oca:
> kfree(oca_blob);
> @@ -503,14 +502,14 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> ret = sev_ioctl_pdh_gen(&input);
> break;
>
> - case SEV_PEK_CSR: {
> + case SEV_PEK_CSR:
> ret = sev_ioctl_pek_csr(&input);
> break;
> - }
> - case SEV_PEK_CERT_IMPORT: {
> +
> + case SEV_PEK_CERT_IMPORT:
> ret = sev_ioctl_pek_cert_import(&input);
> break;
> - }
> +
> default:
> ret = -EINVAL;
> goto out;
>
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Part2 PATCH v5.1 12.9/31] crypto: ccp: Implement SEV_PDH_CERT_EXPORT ioctl command
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
` (6 preceding siblings ...)
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.8/31] crypto: ccp: Implement SEV_PEK_CERT_IMPORT " Brijesh Singh
@ 2017-10-07 1:06 ` Brijesh Singh
2017-10-13 15:01 ` Borislav Petkov
2017-10-07 18:40 ` [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support Borislav Petkov
2017-10-11 16:50 ` [Part2 PATCH v5.2 12.2/31] " Brijesh Singh
9 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-07 1:06 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
The SEV_PDH_CERT_EXPORT command can be used to export the PDH and its
certificate chain. The command is defined in SEV spec section 5.10.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
drivers/crypto/ccp/psp-dev.c | 110 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 110 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 861c44bf2910..0a069e3c7b8c 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -473,6 +473,112 @@ static int sev_ioctl_pek_cert_import(struct sev_issue_cmd *argp)
return ret;
}
+static int sev_ioctl_pdh_cert_export(struct sev_issue_cmd *argp)
+{
+ struct sev_user_data_pdh_cert_export input;
+ struct sev_data_pdh_cert_export *data;
+ int ret, state, need_shutdown = 0;
+ void *pdh_blob, *cert_blob;
+
+ if (copy_from_user(&input, (void __user *)(uintptr_t)argp->data,
+ sizeof(struct sev_user_data_pdh_cert_export)))
+ return -EFAULT;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ pdh_blob = NULL;
+ if (input.pdh_cert_address) {
+ if (!access_ok(VERIFY_WRITE, input.pdh_cert_address, input.pdh_cert_len) ||
+ (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE)) {
+ ret = -EFAULT;
+ goto e_free;
+ }
+
+ pdh_blob = kmalloc(input.pdh_cert_len, GFP_KERNEL);
+ if (!pdh_blob) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ data->pdh_cert_address = __psp_pa(pdh_blob);
+ data->pdh_cert_len = input.pdh_cert_len;
+ }
+
+ cert_blob = NULL;
+ if (input.cert_chain_address) {
+ if (!access_ok(VERIFY_WRITE, input.cert_chain_address, input.cert_chain_len) ||
+ (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE)) {
+ ret = -EFAULT;
+ goto e_free_pdh;
+ }
+
+ cert_blob = kmalloc(input.cert_chain_len, GFP_KERNEL);
+ if (!cert_blob) {
+ ret = -ENOMEM;
+ goto e_free_pdh;
+ }
+
+ data->cert_chain_address = __psp_pa(cert_blob);
+ data->cert_chain_len = input.cert_chain_len;
+ }
+
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ goto e_free_cert;
+
+ /*
+ * CERT_EXPORT command can be issued in INIT or WORKING state.
+ * If we are in UNINIT state then transition to INIT.
+ */
+ if (state == SEV_STATE_UNINIT) {
+ ret = sev_firmware_init(&argp->error);
+ if (ret)
+ goto e_free_cert;
+
+ need_shutdown = 1;
+ }
+
+ ret = sev_handle_cmd(SEV_CMD_PDH_CERT_EXPORT, data, &argp->error);
+
+ input.cert_chain_len = data->cert_chain_len;
+ input.pdh_cert_len = data->pdh_cert_len;
+
+ /* copy certificate length to userspace */
+ if (copy_to_user((void __user *)(uintptr_t)argp->data, &input,
+ sizeof(struct sev_user_data_pdh_cert_export)))
+ ret = -EFAULT;
+
+ if (ret)
+ goto e_shutdown;
+
+ /* copy PDH certificate to userspace */
+ if (pdh_blob &&
+ copy_to_user((void __user *)(uintptr_t)input.pdh_cert_address,
+ pdh_blob, input.pdh_cert_len)) {
+ ret = -EFAULT;
+ goto e_shutdown;
+ }
+
+ /* copy certificate chain to userspace */
+ if (cert_blob &&
+ copy_to_user((void __user *)(uintptr_t)input.cert_chain_address,
+ cert_blob, input.cert_chain_len))
+ ret = -EFAULT;
+
+e_shutdown:
+ if (need_shutdown)
+ sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+e_free_cert:
+ kfree(cert_blob);
+e_free_pdh:
+ kfree(pdh_blob);
+e_free:
+ kfree(data);
+ return ret;
+}
+
static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
{
void __user *argp = (void __user *)arg;
@@ -514,6 +620,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_ioctl_pek_cert_import(&input);
break;
}
+ case SEV_PDH_CERT_EXPORT: {
+ ret = sev_ioctl_pdh_cert_export(&input);
+ break;
+ }
default:
ret = -EINVAL;
break;
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.9/31] crypto: ccp: Implement SEV_PDH_CERT_EXPORT ioctl command
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.9/31] crypto: ccp: Implement SEV_PDH_CERT_EXPORT " Brijesh Singh
@ 2017-10-13 15:01 ` Borislav Petkov
0 siblings, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-13 15:01 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Fri, Oct 06, 2017 at 08:06:07PM -0500, Brijesh Singh wrote:
> The SEV_PDH_CERT_EXPORT command can be used to export the PDH and its
> certificate chain. The command is defined in SEV spec section 5.10.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> drivers/crypto/ccp/psp-dev.c | 110 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 110 insertions(+)
>
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 861c44bf2910..0a069e3c7b8c 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -473,6 +473,112 @@ static int sev_ioctl_pek_cert_import(struct sev_issue_cmd *argp)
> return ret;
> }
>
> +static int sev_ioctl_pdh_cert_export(struct sev_issue_cmd *argp)
> +{
> + struct sev_user_data_pdh_cert_export input;
> + struct sev_data_pdh_cert_export *data;
> + int ret, state, need_shutdown = 0;
> + void *pdh_blob, *cert_blob;
> +
> + if (copy_from_user(&input, (void __user *)(uintptr_t)argp->data,
> + sizeof(struct sev_user_data_pdh_cert_export)))
> + return -EFAULT;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + pdh_blob = NULL;
> + if (input.pdh_cert_address) {
Here...
> + if (!access_ok(VERIFY_WRITE, input.pdh_cert_address, input.pdh_cert_len) ||
> + (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE)) {
> + ret = -EFAULT;
> + goto e_free;
> + }
> +
> + pdh_blob = kmalloc(input.pdh_cert_len, GFP_KERNEL);
> + if (!pdh_blob) {
> + ret = -ENOMEM;
> + goto e_free;
> + }
> +
> + data->pdh_cert_address = __psp_pa(pdh_blob);
> + data->pdh_cert_len = input.pdh_cert_len;
> + }
> +
> + cert_blob = NULL;
> + if (input.cert_chain_address) {
... and here please check the full condition where userspace queries the
cert length, like before.
Otherwise, the usual cleanups:
---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 5e69add4fea9..331d028f9445 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -476,8 +476,7 @@ static int sev_ioctl_pdh_cert_export(struct sev_issue_cmd *argp)
int ret, state, need_shutdown = 0;
void *pdh_blob, *cert_blob;
- if (copy_from_user(&input, (void __user *)(uintptr_t)argp->data,
- sizeof(struct sev_user_data_pdh_cert_export)))
+ if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
data = kzalloc(sizeof(*data), GFP_KERNEL);
@@ -536,36 +535,37 @@ static int sev_ioctl_pdh_cert_export(struct sev_issue_cmd *argp)
need_shutdown = 1;
}
- ret = sev_handle_cmd(SEV_CMD_PDH_CERT_EXPORT, data, &argp->error);
+ ret = sev_do_cmd(SEV_CMD_PDH_CERT_EXPORT, data, &argp->error);
input.cert_chain_len = data->cert_chain_len;
input.pdh_cert_len = data->pdh_cert_len;
/* copy certificate length to userspace */
- if (copy_to_user((void __user *)(uintptr_t)argp->data, &input,
- sizeof(struct sev_user_data_pdh_cert_export)))
+ if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
ret = -EFAULT;
if (ret)
goto e_shutdown;
/* copy PDH certificate to userspace */
- if (pdh_blob &&
- copy_to_user((void __user *)(uintptr_t)input.pdh_cert_address,
- pdh_blob, input.pdh_cert_len)) {
- ret = -EFAULT;
- goto e_shutdown;
+ if (pdh_blob) {
+ if (copy_to_user((void __user *)input.pdh_cert_address,
+ pdh_blob, input.pdh_cert_len)) {
+ ret = -EFAULT;
+ goto e_shutdown;
+ }
}
/* copy certificate chain to userspace */
- if (cert_blob &&
- copy_to_user((void __user *)(uintptr_t)input.cert_chain_address,
- cert_blob, input.cert_chain_len))
- ret = -EFAULT;
+ if (cert_blob) {
+ if (copy_to_user((void __user *)input.cert_chain_address,
+ cert_blob, input.cert_chain_len))
+ ret = -EFAULT;
+ }
e_shutdown:
if (need_shutdown)
- sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+ sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
e_free_cert:
kfree(cert_blob);
e_free_pdh:
@@ -616,10 +616,9 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
ret = sev_ioctl_pek_cert_import(&input);
break;
- case SEV_PDH_CERT_EXPORT: {
+ case SEV_PDH_CERT_EXPORT:
ret = sev_ioctl_pdh_cert_export(&input);
break;
- }
default:
ret = -EINVAL;
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread
* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
` (7 preceding siblings ...)
2017-10-07 1:06 ` [Part2 PATCH v5.1 12.9/31] crypto: ccp: Implement SEV_PDH_CERT_EXPORT " Brijesh Singh
@ 2017-10-07 18:40 ` Borislav Petkov
2017-10-08 13:30 ` Brijesh Singh
2017-10-11 16:50 ` [Part2 PATCH v5.2 12.2/31] " Brijesh Singh
9 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-07 18:40 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Fri, Oct 06, 2017 at 08:05:59PM -0500, Brijesh Singh wrote:
> AMD's new Secure Encrypted Virtualization (SEV) feature allows the
> memory contents of virtual machines to be transparently encrypted with a
> key unique to the VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP) which exposes the
> commands for these tasks. The complete spec is available at:
>
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
>
> Extend the AMD-SP driver to provide the following support:
>
> - an in-kernel API to communicate with the SEV firmware. The API can be
> used by the hypervisor to create encryption context for a SEV guest.
>
> - a userspace IOCTL to manage the platform certificates.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Improvements-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>
> Based on Boris feedback split this patch in 9 logical patches, they are
> numbers from 12.1 to 12.9.
>
> drivers/crypto/ccp/psp-dev.c | 244 +++++++++++++++++++++++++++++++++++++++++++
> drivers/crypto/ccp/psp-dev.h | 17 +++
> include/linux/psp-sev.h | 159 ++++++++++++++++++++++++++++
> 3 files changed, 420 insertions(+)
A bunch of fixes ontop:
* sev_fops_registered is superfluous if you can use psp->has_sev_fops
* s/sev_handle_cmd/sev_do_cmd/g - it really is sev_do_cmd(). "handle" is
something else.
* Flip misc dev reg logic in sev_ops_init()
* PSP_P2CMSG needs arg eval
* text streamlining
---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e9b776c3acb2..f0e0fc1fb512 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -31,7 +31,6 @@
#define DEVICE_NAME "sev"
static DEFINE_MUTEX(sev_cmd_mutex);
-static bool sev_fops_registered;
static struct psp_device *psp_alloc_struct(struct sp_device *sp)
{
@@ -121,7 +120,7 @@ static int sev_cmd_buffer_len(int cmd)
return 0;
}
-static int sev_handle_cmd(int cmd, void *data, int *psp_ret)
+static int sev_do_cmd(int cmd, void *data, int *psp_ret)
{
unsigned int phys_lsb, phys_msb;
struct psp_device *psp;
@@ -182,26 +181,26 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
return -ENOTTY;
}
-const struct file_operations sev_fops = {
+static const struct file_operations sev_fops = {
.owner = THIS_MODULE,
.unlocked_ioctl = sev_ioctl,
};
int sev_platform_init(struct sev_data_init *data, int *error)
{
- return sev_handle_cmd(SEV_CMD_INIT, data, error);
+ return sev_do_cmd(SEV_CMD_INIT, data, error);
}
EXPORT_SYMBOL_GPL(sev_platform_init);
int sev_platform_shutdown(int *error)
{
- return sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, error);
+ return sev_do_cmd(SEV_CMD_SHUTDOWN, 0, error);
}
EXPORT_SYMBOL_GPL(sev_platform_shutdown);
int sev_platform_status(struct sev_data_status *data, int *error)
{
- return sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+ return sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
}
EXPORT_SYMBOL_GPL(sev_platform_status);
@@ -211,58 +210,58 @@ int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
if (!filep || filep->f_op != &sev_fops)
return -EBADF;
- return sev_handle_cmd(cmd, data, error);
+ return sev_do_cmd(cmd, data, error);
}
EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
int sev_guest_deactivate(struct sev_data_deactivate *data, int *error)
{
- return sev_handle_cmd(SEV_CMD_DEACTIVATE, data, error);
+ return sev_do_cmd(SEV_CMD_DEACTIVATE, data, error);
}
EXPORT_SYMBOL_GPL(sev_guest_deactivate);
int sev_guest_activate(struct sev_data_activate *data, int *error)
{
- return sev_handle_cmd(SEV_CMD_ACTIVATE, data, error);
+ return sev_do_cmd(SEV_CMD_ACTIVATE, data, error);
}
EXPORT_SYMBOL_GPL(sev_guest_activate);
int sev_guest_decommission(struct sev_data_decommission *data, int *error)
{
- return sev_handle_cmd(SEV_CMD_DECOMMISSION, data, error);
+ return sev_do_cmd(SEV_CMD_DECOMMISSION, data, error);
}
EXPORT_SYMBOL_GPL(sev_guest_decommission);
int sev_guest_df_flush(int *error)
{
- return sev_handle_cmd(SEV_CMD_DF_FLUSH, 0, error);
+ return sev_do_cmd(SEV_CMD_DF_FLUSH, 0, error);
}
EXPORT_SYMBOL_GPL(sev_guest_df_flush);
static int sev_ops_init(struct psp_device *psp)
{
struct miscdevice *misc = &psp->sev_misc;
- int ret = 0;
+ int ret;
/*
- * SEV feature support can be detected on the multiple devices but the
- * SEV FW commands must be issued on the master. During probe time we
- * do not know the master hence we create /dev/sev on the first device
- * probe. sev_handle_cmd() finds the right master device to when issuing
- * the command to the firmware.
+ * SEV feature support can be detected on multiple devices but the SEV
+ * FW commands must be issued on the master. During probe, we do not
+ * know the master hence we create /dev/sev on the first device probe.
+ * sev_do_cmd() finds the right master device to which to issue the
+ * command to the firmware.
*/
- if (!sev_fops_registered) {
- misc->minor = MISC_DYNAMIC_MINOR;
- misc->name = DEVICE_NAME;
- misc->fops = &sev_fops;
-
- ret = misc_register(misc);
- if (!ret) {
- sev_fops_registered = true;
- psp->has_sev_fops = true;
- init_waitqueue_head(&psp->sev_int_queue);
- dev_info(psp->dev, "registered SEV device\n");
- }
+ if (psp->has_sev_fops)
+ return 0;
+
+ misc->minor = MISC_DYNAMIC_MINOR;
+ misc->name = DEVICE_NAME;
+ misc->fops = &sev_fops;
+
+ ret = misc_register(misc);
+ if (!ret) {
+ psp->has_sev_fops = true;
+ init_waitqueue_head(&psp->sev_int_queue);
+ dev_info(psp->dev, "registered SEV device\n");
}
return ret;
diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h
index 6e8f83b41521..60a33f5ff79f 100644
--- a/drivers/crypto/ccp/psp-dev.h
+++ b/drivers/crypto/ccp/psp-dev.h
@@ -36,7 +36,7 @@
#define PSP_CMDBUFF_ADDR_HI PSP_C2PMSG(57)
#define PSP_FEATURE_REG PSP_C2PMSG(63)
-#define PSP_P2CMSG(_num) (_num << 2)
+#define PSP_P2CMSG(_num) ((_num) << 2)
#define PSP_CMD_COMPLETE_REG 1
#define PSP_CMD_COMPLETE PSP_P2CMSG(PSP_CMD_COMPLETE_REG)
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 2b334fd853c9..10b843cce75f 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -512,8 +512,7 @@ struct sev_data_dbg {
u32 len; /* In */
} __packed;
-#if defined(CONFIG_CRYPTO_DEV_SP_PSP)
-
+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
/**
* sev_platform_init - perform SEV INIT command
*
@@ -562,9 +561,9 @@ int sev_platform_status(struct sev_data_status *status, int *error);
* sev_issue_cmd_external_user - issue SEV command by other driver with a file
* handle.
*
- * The function can be used by other drivers to issue a SEV command on
- * behalf by userspace. The caller must pass a valid SEV file descriptor
- * so that we know that caller has access to SEV device.
+ * This function can be used by other drivers to issue a SEV command on
+ * behalf of userspace. The caller must pass a valid SEV file descriptor
+ * so that we know that it has access to SEV device.
*
* @filep - SEV device file pointer
* @cmd - command to issue
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-07 18:40 ` [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support Borislav Petkov
@ 2017-10-08 13:30 ` Brijesh Singh
2017-10-08 14:00 ` Borislav Petkov
2017-10-11 14:19 ` Borislav Petkov
0 siblings, 2 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-08 13:30 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/7/17 1:40 PM, Borislav Petkov wrote:
...
> A bunch of fixes ontop:
>
> * sev_fops_registered is superfluous if you can use psp->has_sev_fops
I am okay with all your fixes except this one. I will add my comment below.
...
> static int sev_ops_init(struct psp_device *psp)
> {
> struct miscdevice *misc = &psp->sev_misc;
> - int ret = 0;
> + int ret;
>
> /*
> - * SEV feature support can be detected on the multiple devices but the
> - * SEV FW commands must be issued on the master. During probe time we
> - * do not know the master hence we create /dev/sev on the first device
> - * probe. sev_handle_cmd() finds the right master device to when issuing
> - * the command to the firmware.
> + * SEV feature support can be detected on multiple devices but the SEV
> + * FW commands must be issued on the master. During probe, we do not
> + * know the master hence we create /dev/sev on the first device probe.
> + * sev_do_cmd() finds the right master device to which to issue the
> + * command to the firmware.
> */
> - if (!sev_fops_registered) {
> - misc->minor = MISC_DYNAMIC_MINOR;
> - misc->name = DEVICE_NAME;
> - misc->fops = &sev_fops;
> -
> - ret = misc_register(misc);
> - if (!ret) {
> - sev_fops_registered = true;
> - psp->has_sev_fops = true;
> - init_waitqueue_head(&psp->sev_int_queue);
> - dev_info(psp->dev, "registered SEV device\n");
> - }
> + if (psp->has_sev_fops)
> + return 0;
> +
This will always be false. The struct psp_device is used for storing
per-device instance.
> + misc->minor = MISC_DYNAMIC_MINOR;
> + misc->name = DEVICE_NAME;
> + misc->fops = &sev_fops;
> +
> + ret = misc_register(misc);
> + if (!ret) {
> + psp->has_sev_fops = true;
> + init_waitqueue_head(&psp->sev_int_queue);
> + dev_info(psp->dev, "registered SEV device\n");
> }
During the device probe, sev_ops_init() will be called for every device
instance which claims to support the SEV. One of the device will be
'master' but we don't the master until we probe all the instances. Hence
the probe for all SEV devices must return success.
With your changes, the first device instance will able to create
/dev/sev node but all other instances will fail hence the probe routine
for other instances will also fail.
Basically we need some variable which is outside the per-device
structure so that we don't end up creating multiple /dev/sev nodes. If
needed, I think we can remove 'has_sev_fops' variable from struct
psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in
struct psp_device. The 'has_sev_fops' is mainly used in sev_exit(). If
we decide to dynamic alloc sev_misc then in sev_exit() we can use
psp->sev_misc != NULL instead of psp->has_sev_ops.
>
> return ret;
> diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h
> index 6e8f83b41521..60a33f5ff79f 100644
> --- a/drivers/crypto/ccp/psp-dev.h
> +++ b/drivers/crypto/ccp/psp-dev.h
> @@ -36,7 +36,7 @@
> #define PSP_CMDBUFF_ADDR_HI PSP_C2PMSG(57)
> #define PSP_FEATURE_REG PSP_C2PMSG(63)
>
> -#define PSP_P2CMSG(_num) (_num << 2)
> +#define PSP_P2CMSG(_num) ((_num) << 2)
> #define PSP_CMD_COMPLETE_REG 1
> #define PSP_CMD_COMPLETE PSP_P2CMSG(PSP_CMD_COMPLETE_REG)
>
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 2b334fd853c9..10b843cce75f 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -512,8 +512,7 @@ struct sev_data_dbg {
> u32 len; /* In */
> } __packed;
>
> -#if defined(CONFIG_CRYPTO_DEV_SP_PSP)
> -
> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
> /**
> * sev_platform_init - perform SEV INIT command
> *
> @@ -562,9 +561,9 @@ int sev_platform_status(struct sev_data_status *status, int *error);
> * sev_issue_cmd_external_user - issue SEV command by other driver with a file
> * handle.
> *
> - * The function can be used by other drivers to issue a SEV command on
> - * behalf by userspace. The caller must pass a valid SEV file descriptor
> - * so that we know that caller has access to SEV device.
> + * This function can be used by other drivers to issue a SEV command on
> + * behalf of userspace. The caller must pass a valid SEV file descriptor
> + * so that we know that it has access to SEV device.
> *
> * @filep - SEV device file pointer
> * @cmd - command to issue
>
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-08 13:30 ` Brijesh Singh
@ 2017-10-08 14:00 ` Borislav Petkov
2017-10-09 0:11 ` Brijesh Singh
2017-10-11 14:19 ` Borislav Petkov
1 sibling, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-08 14:00 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
> During the device probe, sev_ops_init() will be called for every device
> instance which claims to support the SEV. One of the device will be
> 'master' but we don't the master until we probe all the instances. Hence
> the probe for all SEV devices must return success.
I still am wondering whether that design with multiple devices - master
and non-master devices is optimal. Why isn't the security processor a
single driver which provides the whole functionality, PSP including? Why
do you need all that register and unregister glue and get_master bla if
you can simply put the whole thing in a single module?
And the fact that you need a global variable to mark that you've
registered the misc device should already tell you that something's
not optimal. Because if you had a single driver, it will go, detect
the whole functionality, initialize it, register services and be done
with it. No registering of devices, no finding of masters, no global
variables, no unnecessary glue.
IOW, in this diagram:
+--- CCP
|
AMD-SP --|
| +--- SEV
| |
+---- PSP ---*
|
+---- TEE
why isn't the whole PSP functionality part of drivers/crypto/ccp/sp-dev.c ?
That driver is almost barebones minimal thing. You can very well add the
PSP/SEV stuff in there and if there's an *actual* reason to carve pieces
out, only then to do so, not to split it now unnecessarily and make your
life complicated for no reason.
Or am I missing some obvious and important reason?
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-08 14:00 ` Borislav Petkov
@ 2017-10-09 0:11 ` Brijesh Singh
2017-10-09 15:21 ` Borislav Petkov
0 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-09 0:11 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/8/17 9:00 AM, Borislav Petkov wrote:
> On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
>> During the device probe, sev_ops_init() will be called for every device
>> instance which claims to support the SEV. One of the device will be
>> 'master' but we don't the master until we probe all the instances. Hence
>> the probe for all SEV devices must return success.
> I still am wondering whether that design with multiple devices - master
> and non-master devices is optimal. Why isn't the security processor a
> single driver which provides the whole functionality, PSP including? Why
> do you need all that register and unregister glue and get_master bla if
> you can simply put the whole thing in a single module?
There is a single security processor driver (ccp) which provides the
complete functionality including PSP. But the driver should be able to
work with multiple devices. e.g In my 2P EPYC configuration, security
processor driver is used for the following devices
02:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
14:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
22:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
23:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
31:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
32:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
41:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
42:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
51:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
52:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
61:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
62:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
71:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
72:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
Some of the these devices support CCP only and some support both PSP and
CCP. It's possible that multiple devices support the PSP functionality
but only one of them is master which can be used for issuing SEV
commands. There isn't a register which we can read to determine whether
the device is master. We have to probe all the devices which supports
the PSP to determine which one of them is a master.
> And the fact that you need a global variable to mark that you've
> registered the misc device should already tell you that something's
> not optimal. Because if you had a single driver, it will go, detect
> the whole functionality, initialize it, register services and be done
> with it. No registering of devices, no finding of masters, no global
> variables, no unnecessary glue.
>
> IOW, in this diagram:
>
> +--- CCP
> |
> AMD-SP --|
> | +--- SEV
> | |
> +---- PSP ---*
> |
> +---- TEE
>
> why isn't the whole PSP functionality part of drivers/crypto/ccp/sp-dev.c ?
>
> That driver is almost barebones minimal thing. You can very well add the
> PSP/SEV stuff in there and if there's an *actual* reason to carve pieces
> out, only then to do so, not to split it now unnecessarily and make your
> life complicated for no reason.
I was trying to follow the CCP model -- in which sp-dev.c simply
forwards the call to ccp-dev.c which does the real work. Currently,
sev-dev.c contains barebone common code. IMO, keeping all the PSP
private functions and data structure outside the sp-dev.c/.h is right
thing. Having said that, I am okay with moving all the PSP stuff in
sp-dev.c but before we do that lets see what CCP maintainers say.
Tom and Gary, comments please ?
Additionally, I would like to highlight that if we decide to go with
moving all the PSP functionality in sp-dev.c then we have to add #ifdef
CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas
the sp-dev.c gets compiled for all architectures (including aarch64,
i386 and x86_64).
>
> Or am I missing some obvious and important reason?
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-09 0:11 ` Brijesh Singh
@ 2017-10-09 15:21 ` Borislav Petkov
2017-10-10 15:00 ` Brijesh Singh
0 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-09 15:21 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Sun, Oct 08, 2017 at 07:11:04PM -0500, Brijesh Singh wrote:
> There is a single security processor driver (ccp) which provides the
> complete functionality including PSP. But the driver should be able to
> work with multiple devices. e.g In my 2P EPYC configuration, security
> processor driver is used for the following devices
>
> 02:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
> 1456
So we have a lot of drivers which claim more than one PCI device. It is
not mandatory to have a driver per PCI device. Actually, the decisive
argument is what is the easiest way to manage those devices and what
makes the communication between them the easiest.
> 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
> 1468
> 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
> 1456
Btw, what do those PCI functions each do? Public PPR doesn't have them
documented.
> Some of the these devices support CCP only and some support both PSP and
> CCP. It's possible that multiple devices support the PSP functionality
> but only one of them is master which can be used for issuing SEV
> commands. There isn't a register which we can read to determine whether
> the device is master. We have to probe all the devices which supports
> the PSP to determine which one of them is a master.
Sure, and if you manage all the devices in a single driver, you can
simply keep them all in a linked list or in an array and iterating over
them is trivial.
Because right now you have
1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection
2. at some point, it does sp-dev.c::sp_init() which decides whether CCP or PSP
3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that
sp->dev_vdata->psp_vdata which is nothing more than a simple offset
0x10500 which is where the PSP io regs are. For example, if this offset
is hardcoded, why are we even passing that vdata? Just set psp->io_regs =
0x10500. No need for all that passing of structs around.
4. and finally, after that *whole* init has been done, you get to do
->set_psp_master_device(sp);
Or, you can save yourself all that jumping through hoops, merge sp-pci.c
and sp-dev.c into a single sp.c and put *everything* sp-related into
it. And then do the whole work of picking hw apart, detection and
configuration in sp_pci_probe() and have helper functions preparing and
configuring the device.
At the end, it adds it to the list of devices sp.c manages and done. You
actually have that list already:
static LIST_HEAD(sp_units);
in sp-dev.c.
You don't need the set_master thing either - you simply set the
sp_dev_master pointer inside sp.c
sp_init() can then go and you can replace it with its function body,
deciding whether it is a CCP or PSP and then call the respective
function which is also in sp.c or ccp-dev.c
And then all those separate compilation units and the interfaces between
them disappear - you have only the calls into the PSP and that's it.
Btw, the CCP thing could remain separate initially, I guess, with all
that ccp-* stuff in there.
> I was trying to follow the CCP model -- in which sp-dev.c simply
> forwards the call to ccp-dev.c which does the real work.
And you don't really need that - you can do the real work directly in
sp-dev.c or sp.c or whatever.
> Currently, sev-dev.c contains barebone common code. IMO, keeping all
> the PSP private functions and data structure outside the sp-dev.c/.h
> is right thing.
By this model probably, but it causes all that init and registration
jump-through-hoops for no real reason. It is basically wasting cycles
and energy.
I'm all for splitting if it makes sense. But right now I don't see much
sense in this - it is basically a bunch of small compilation units
calling each other. And they could be merged into a single sp.c which
does it all in one go, without a lot of blabla.
> Additionally, I would like to highlight that if we decide to go with
> moving all the PSP functionality in sp-dev.c then we have to add #ifdef
> CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas
> the sp-dev.c gets compiled for all architectures (including aarch64,
> i386 and x86_64).
That's fine. You can build it on 32-bit but add to the init function
if (IS_ENABLED(CONFIG_X86_32))
return -ENODEV;
and be done with it. No need for the ifdeffery.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-09 15:21 ` Borislav Petkov
@ 2017-10-10 15:00 ` Brijesh Singh
2017-10-10 18:43 ` Tom Lendacky
0 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-10 15:00 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/09/2017 10:21 AM, Borislav Petkov wrote:
...
>
>> 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
>> 1468
>> 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
>> 1456
>
> Btw, what do those PCI functions each do? Public PPR doesn't have them
> documented.
Looking at the pci_device_id table (sp-pci.c), the devices id 0x1468
provides the support CCP support directly on the x86-side and device id
0x1456 provides the support for both CCP and PSP features through the
AMD Secure Processor (AMD-SP).
>
> Sure, and if you manage all the devices in a single driver, you can
> simply keep them all in a linked list or in an array and iterating over
> them is trivial.
>
> Because right now you have
>
> 1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection
>
> 2. at some point, it does sp-dev.c::sp_init() which decides whether CCP or PSP
>
> 3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that
> sp->dev_vdata->psp_vdata which is nothing more than a simple offset
> 0x10500 which is where the PSP io regs are. For example, if this offset
> is hardcoded, why are we even passing that vdata? Just set psp->io_regs =
> 0x10500. No need for all that passing of structs around.
>
> 4. and finally, after that *whole* init has been done, you get to do
> ->set_psp_master_device(sp);
>
> Or, you can save yourself all that jumping through hoops, merge sp-pci.c
> and sp-dev.c into a single sp.c and put *everything* sp-related into
> it. And then do the whole work of picking hw apart, detection and
> configuration in sp_pci_probe() and have helper functions preparing and
> configuring the device.
>
> At the end, it adds it to the list of devices sp.c manages and done. You
> actually have that list already:
>
> static LIST_HEAD(sp_units);
>
> in sp-dev.c.
>
> You don't need the set_master thing either - you simply set the
> sp_dev_master pointer inside sp.c
>
I was trying to avoid putting PSP/SEV specific changes in sp-dev.*
files. But if sp.c approach is acceptable to the maintainer then I can
work towards merging sp-dev.c and sp-pci.c into sp.c and then add the
PSP/SEV support.
> sp_init() can then go and you can replace it with its function body,
> deciding whether it is a CCP or PSP and then call the respective
> function which is also in sp.c or ccp-dev.c
>
> And then all those separate compilation units and the interfaces between
> them disappear - you have only the calls into the PSP and that's it.
>
> Btw, the CCP thing could remain separate initially, I guess, with all
> that ccp-* stuff in there.
>
Yep, if we decide to go with your recommended approach then we should
leave the CCP as-is for now.
>> I was trying to follow the CCP model -- in which sp-dev.c simply
>> forwards the call to ccp-dev.c which does the real work.
>
> And you don't really need that - you can do the real work directly in
> sp-dev.c or sp.c or whatever.
> >> Currently, sev-dev.c contains barebone common code. IMO, keeping all
>> the PSP private functions and data structure outside the sp-dev.c/.h
>> is right thing.
>
> By this model probably, but it causes all that init and registration
> jump-through-hoops for no real reason. It is basically wasting cycles
> and energy.
>
> I'm all for splitting if it makes sense. But right now I don't see much
> sense in this - it is basically a bunch of small compilation units
> calling each other. And they could be merged into a single sp.c which
> does it all in one go, without a lot of blabla.
>> Additionally, I would like to highlight that if we decide to go with
>> moving all the PSP functionality in sp-dev.c then we have to add #ifdef
>> CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas
>> the sp-dev.c gets compiled for all architectures (including aarch64,
>> i386 and x86_64).
>
> That's fine. You can build it on 32-bit but add to the init function
>
> if (IS_ENABLED(CONFIG_X86_32))
> return -ENODEV;
>
> and be done with it. No need for the ifdeffery.
>
OK, i will use IS_ENABLED where applicable.
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-10 15:00 ` Brijesh Singh
@ 2017-10-10 18:43 ` Tom Lendacky
2017-10-10 20:04 ` Borislav Petkov
0 siblings, 1 reply; 74+ messages in thread
From: Tom Lendacky @ 2017-10-10 18:43 UTC (permalink / raw)
To: Brijesh Singh, Borislav Petkov
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
linux-crypto, kvm, linux-kernel
On 10/10/2017 10:00 AM, Brijesh Singh wrote:
>
>
> On 10/09/2017 10:21 AM, Borislav Petkov wrote:
> ...
>
>>
>>> 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
>>> 1468
>>> 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
>>> 1456
>>
>> Btw, what do those PCI functions each do? Public PPR doesn't have them
>> documented.
>
>
> Looking at the pci_device_id table (sp-pci.c), the devices id 0x1468
> provides the support CCP support directly on the x86-side and device id
> 0x1456 provides the support for both CCP and PSP features through the AMD
> Secure Processor (AMD-SP).
>
>
>>
>> Sure, and if you manage all the devices in a single driver, you can
>> simply keep them all in a linked list or in an array and iterating over
>> them is trivial.
>>
>> Because right now you have
>>
>> 1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection
>>
>> 2. at some point, it does sp-dev.c::sp_init() which decides whether CCP
>> or PSP
>>
>> 3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that
>> sp->dev_vdata->psp_vdata which is nothing more than a simple offset
>> 0x10500 which is where the PSP io regs are. For example, if this offset
>> is hardcoded, why are we even passing that vdata? Just set psp->io_regs =
>> 0x10500. No need for all that passing of structs around.
Maybe for the very first implementation we could do that and that was what
was originally done for the CCP. But as you can see the CCP does not have
a set register offset between various iterations of the device and it can
be expected the same will hold true for the PSP. This just makes future
changes easier in order to support newer devices.
>>
>> 4. and finally, after that *whole* init has been done, you get to do
>> ->set_psp_master_device(sp);
>>
>> Or, you can save yourself all that jumping through hoops, merge sp-pci.c
>> and sp-dev.c into a single sp.c and put *everything* sp-related into
>> it. And then do the whole work of picking hw apart, detection and
>> configuration in sp_pci_probe() and have helper functions preparing and
>> configuring the device.
>>
>> At the end, it adds it to the list of devices sp.c manages and done. You
>> actually have that list already:
>>
>> static LIST_HEAD(sp_units);
>>
>> in sp-dev.c.
>>
>> You don't need the set_master thing either - you simply set the
>> sp_dev_master pointer inside sp.c
>>
>
>
> I was trying to avoid putting PSP/SEV specific changes in sp-dev.* files.
> But if sp.c approach is acceptable to the maintainer then I can work
> towards merging sp-dev.c and sp-pci.c into sp.c and then add the PSP/SEV
> support.
I would prefer to keep things separated as they are. The common code is
one place and the pci/platform specific code resides in unique files. For
sp-pci.c, it can be excluded from compilation if CONFIG_PCI is not defined
vs. adding #ifdefs into sp-dev.c or sp.c. The code is working nicely and,
at least to me, seems easily maintainable this way. If we really want to
avoid the extra calls during probing, etc. then we can take a closer look
afterwards and determine what is the best approach taking into account
the CCP and some of the other PSP functionality that is coming.
Thanks,
Tom
>
>
>> sp_init() can then go and you can replace it with its function body,
>> deciding whether it is a CCP or PSP and then call the respective
>> function which is also in sp.c or ccp-dev.c
>>
>> And then all those separate compilation units and the interfaces between
>> them disappear - you have only the calls into the PSP and that's it.
>>
>> Btw, the CCP thing could remain separate initially, I guess, with all
>> that ccp-* stuff in there.
>>
>
> Yep, if we decide to go with your recommended approach then we should
> leave the CCP as-is for now.
>
>
>>> I was trying to follow the CCP model -- in which sp-dev.c simply
>>> forwards the call to ccp-dev.c which does the real work.
>>
>> And you don't really need that - you can do the real work directly in
>> sp-dev.c or sp.c or whatever.
>> >> Currently, sev-dev.c contains barebone common code. IMO, keeping all
>>> the PSP private functions and data structure outside the sp-dev.c/.h
>>> is right thing.
>>
>> By this model probably, but it causes all that init and registration
>> jump-through-hoops for no real reason. It is basically wasting cycles
>> and energy.
>>
>> I'm all for splitting if it makes sense. But right now I don't see much
>> sense in this - it is basically a bunch of small compilation units
>> calling each other. And they could be merged into a single sp.c which
>> does it all in one go, without a lot of blabla.
>
>>> Additionally, I would like to highlight that if we decide to go with
>>> moving all the PSP functionality in sp-dev.c then we have to add #ifdef
>>> CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas
>>> the sp-dev.c gets compiled for all architectures (including aarch64,
>>> i386 and x86_64).
>>
>> That's fine. You can build it on 32-bit but add to the init function
>>
>> if (IS_ENABLED(CONFIG_X86_32))
>> return -ENODEV;
>>
>> and be done with it. No need for the ifdeffery.
>>
>
> OK, i will use IS_ENABLED where applicable.
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-10 18:43 ` Tom Lendacky
@ 2017-10-10 20:04 ` Borislav Petkov
0 siblings, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-10 20:04 UTC (permalink / raw)
To: Tom Lendacky
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, linux-crypto, kvm, linux-kernel
On Tue, Oct 10, 2017 at 01:43:22PM -0500, Tom Lendacky wrote:
> Maybe for the very first implementation we could do that and that was what
> was originally done for the CCP. But as you can see the CCP does not have
> a set register offset between various iterations of the device and it can
> be expected the same will hold true for the PSP. This just makes future
> changes easier in order to support newer devices.
Well, that's a simple switch-case statement which maps device iteration
to offset.
> I would prefer to keep things separated as they are. The common code is
> one place and the pci/platform specific code resides in unique files. For
> sp-pci.c, it can be excluded from compilation if CONFIG_PCI is not defined
> vs. adding #ifdefs into sp-dev.c or sp.c. The code is working nicely and,
> at least to me, seems easily maintainable this way.
But you do see that you're doing a bunch of unnecessary things during
probing. And all those different devices: SP, CCP, PSP, TEE and whatnot,
they're just too granulary and it is a bunch of registration code and
one compilation unit calling into the other for no good reason. Or at
least I don't see it.
> If we really want to avoid the extra calls during probing, etc.
> then we can take a closer look afterwards and determine what is the
> best approach taking into account the CCP and some of the other PSP
> functionality that is coming.
And that coming functionality won't make things easier - you'll most
likely have more things wanting to talk to more other things. But ok,
your call. Just note that changing things later is always harder.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-08 13:30 ` Brijesh Singh
2017-10-08 14:00 ` Borislav Petkov
@ 2017-10-11 14:19 ` Borislav Petkov
2017-10-11 14:23 ` Brijesh Singh
1 sibling, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-11 14:19 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
> Basically we need some variable which is outside the per-device
> structure so that we don't end up creating multiple /dev/sev nodes. If
> needed, I think we can remove 'has_sev_fops' variable from struct
> psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in
> struct psp_device. The 'has_sev_fops' is mainly used in sev_exit(). If
> we decide to dynamic alloc sev_misc then in sev_exit() we can use
> psp->sev_misc != NULL instead of psp->has_sev_ops.
Yap, that sounds better than an explicit variable.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-11 14:19 ` Borislav Petkov
@ 2017-10-11 14:23 ` Brijesh Singh
0 siblings, 0 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-11 14:23 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/11/2017 09:19 AM, Borislav Petkov wrote:
> On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
>> Basically we need some variable which is outside the per-device
>> structure so that we don't end up creating multiple /dev/sev nodes. If
>> needed, I think we can remove 'has_sev_fops' variable from struct
>> psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in
>> struct psp_device. The 'has_sev_fops' is mainly used in sev_exit(). If
>> we decide to dynamic alloc sev_misc then in sev_exit() we can use
>> psp->sev_misc != NULL instead of psp->has_sev_ops.
>
> Yap, that sounds better than an explicit variable.
>
Sure, I will implement it and send you v5.2. thanks
^ permalink raw reply [flat|nested] 74+ messages in thread
* [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-07 1:05 ` [Part2 PATCH v5.1 12.1/31] " Brijesh Singh
` (8 preceding siblings ...)
2017-10-07 18:40 ` [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support Borislav Petkov
@ 2017-10-11 16:50 ` Brijesh Singh
2017-10-12 14:08 ` Borislav Petkov
2017-10-12 18:21 ` Borislav Petkov
9 siblings, 2 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-11 16:50 UTC (permalink / raw)
To: bp
Cc: Brijesh Singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
AMD's new Secure Encrypted Virtualization (SEV) feature allows the
memory contents of virtual machines to be transparently encrypted with a
key unique to the VM. The programming and management of the encryption
keys are handled by the AMD Secure Processor (AMD-SP) which exposes the
commands for these tasks. The complete spec is available at:
http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
Extend the AMD-SP driver to provide the following support:
- an in-kernel API to communicate with the SEV firmware. The API can be
used by the hypervisor to create encryption context for a SEV guest.
- a userspace IOCTL to manage the platform certificates.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: linux-crypto@vger.kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Improvements-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
Make it as a second patch in the series (changes from 12.1 -> 12.2)
Changes since v5.1:
* text streamlining (from Boris)
* rename sev_handle_cmd -> sev_do_cmd (from Boris)
* PSP_P2CMSG needs arg eval (from Boris)
* use #ifdef instead of #if defined() (from Boris)
drivers/crypto/ccp/psp-dev.c | 251 +++++++++++++++++++++++++++++++++++++++++++
drivers/crypto/ccp/psp-dev.h | 16 +++
include/linux/psp-sev.h | 159 +++++++++++++++++++++++++++
3 files changed, 426 insertions(+)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index b5789f878560..175cb3c3b8ef 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -23,9 +23,16 @@
#include <linux/hw_random.h>
#include <linux/ccp.h>
+#include <uapi/linux/psp-sev.h>
+
#include "sp-dev.h"
#include "psp-dev.h"
+#define DEVICE_NAME "sev"
+
+static DEFINE_MUTEX(sev_cmd_mutex);
+static bool sev_fops_registered;
+
static struct psp_device *psp_alloc_struct(struct sp_device *sp)
{
struct device *dev = sp->dev;
@@ -45,9 +52,246 @@ static struct psp_device *psp_alloc_struct(struct sp_device *sp)
static irqreturn_t psp_irq_handler(int irq, void *data)
{
+ struct psp_device *psp = data;
+ unsigned int status;
+ int reg;
+
+ /* Read the interrupt status: */
+ status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS);
+
+ /* Check if it is command completion: */
+ if (!(status & BIT(PSP_CMD_COMPLETE_REG)))
+ goto done;
+
+ /* Check if it is SEV command completion: */
+ reg = ioread32(psp->io_regs + PSP_CMDRESP);
+ if (reg & PSP_CMDRESP_RESP) {
+ psp->sev_int_rcvd = 1;
+ wake_up(&psp->sev_int_queue);
+ }
+
+done:
+ /* Clear the interrupt status by writing the same value we read. */
+ iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS);
+
return IRQ_HANDLED;
}
+static int sev_wait_cmd_ioc(struct psp_device *psp, unsigned int *reg)
+{
+ psp->sev_int_rcvd = 0;
+
+ wait_event(psp->sev_int_queue, psp->sev_int_rcvd);
+ *reg = ioread32(psp->io_regs + PSP_CMDRESP);
+
+ return 0;
+}
+
+static int sev_cmd_buffer_len(int cmd)
+{
+ switch (cmd) {
+ case SEV_CMD_INIT: return sizeof(struct sev_data_init);
+ case SEV_CMD_PLATFORM_STATUS: return sizeof(struct sev_data_status);
+ case SEV_CMD_PEK_CSR: return sizeof(struct sev_data_pek_csr);
+ case SEV_CMD_PEK_CERT_IMPORT: return sizeof(struct sev_data_pek_cert_import);
+ case SEV_CMD_PDH_CERT_EXPORT: return sizeof(struct sev_data_pdh_cert_export);
+ case SEV_CMD_LAUNCH_START: return sizeof(struct sev_data_launch_start);
+ case SEV_CMD_LAUNCH_UPDATE_DATA: return sizeof(struct sev_data_launch_update_data);
+ case SEV_CMD_LAUNCH_UPDATE_VMSA: return sizeof(struct sev_data_launch_update_vmsa);
+ case SEV_CMD_LAUNCH_FINISH: return sizeof(struct sev_data_launch_finish);
+ case SEV_CMD_LAUNCH_MEASURE: return sizeof(struct sev_data_launch_measure);
+ case SEV_CMD_ACTIVATE: return sizeof(struct sev_data_activate);
+ case SEV_CMD_DEACTIVATE: return sizeof(struct sev_data_deactivate);
+ case SEV_CMD_DECOMMISSION: return sizeof(struct sev_data_decommission);
+ case SEV_CMD_GUEST_STATUS: return sizeof(struct sev_data_guest_status);
+ case SEV_CMD_DBG_DECRYPT: return sizeof(struct sev_data_dbg);
+ case SEV_CMD_DBG_ENCRYPT: return sizeof(struct sev_data_dbg);
+ case SEV_CMD_SEND_START: return sizeof(struct sev_data_send_start);
+ case SEV_CMD_SEND_UPDATE_DATA: return sizeof(struct sev_data_send_update_data);
+ case SEV_CMD_SEND_UPDATE_VMSA: return sizeof(struct sev_data_send_update_vmsa);
+ case SEV_CMD_SEND_FINISH: return sizeof(struct sev_data_send_finish);
+ case SEV_CMD_RECEIVE_START: return sizeof(struct sev_data_receive_start);
+ case SEV_CMD_RECEIVE_FINISH: return sizeof(struct sev_data_receive_finish);
+ case SEV_CMD_RECEIVE_UPDATE_DATA: return sizeof(struct sev_data_receive_update_data);
+ case SEV_CMD_RECEIVE_UPDATE_VMSA: return sizeof(struct sev_data_receive_update_vmsa);
+ case SEV_CMD_LAUNCH_UPDATE_SECRET: return sizeof(struct sev_data_launch_secret);
+ default: return 0;
+ }
+
+ return 0;
+}
+
+static int sev_do_cmd(int cmd, void *data, int *psp_ret)
+{
+ unsigned int phys_lsb, phys_msb;
+ struct psp_device *psp;
+ unsigned int reg, ret;
+ struct sp_device *sp;
+
+ sp = sp_get_psp_master_device();
+ if (!sp)
+ return -ENODEV;
+
+ psp = sp->psp_data;
+ if (!psp)
+ return -ENODEV;
+
+ /* Get the physical address of the command buffer */
+ phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
+ phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0;
+
+ dev_dbg(psp->dev, "sev command id %#x buffer 0x%08x%08x\n",
+ cmd, phys_msb, phys_lsb);
+
+ print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
+ sev_cmd_buffer_len(cmd), false);
+
+ /* Only one command at a time... */
+ mutex_lock(&sev_cmd_mutex);
+
+ iowrite32(phys_lsb, psp->io_regs + PSP_CMDBUFF_ADDR_LO);
+ iowrite32(phys_msb, psp->io_regs + PSP_CMDBUFF_ADDR_HI);
+
+ reg = cmd;
+ reg <<= PSP_CMDRESP_CMD_SHIFT;
+ reg |= PSP_CMDRESP_IOC;
+ iowrite32(reg, psp->io_regs + PSP_CMDRESP);
+
+ ret = sev_wait_cmd_ioc(psp, ®);
+ if (ret)
+ goto unlock;
+
+ if (psp_ret)
+ *psp_ret = reg & PSP_CMDRESP_ERR_MASK;
+
+ if (reg & PSP_CMDRESP_ERR_MASK) {
+ dev_dbg(psp->dev, "sev command %#x failed (%#010x)\n",
+ cmd, reg & PSP_CMDRESP_ERR_MASK);
+ ret = -EIO;
+ }
+
+unlock:
+ mutex_unlock(&sev_cmd_mutex);
+ print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
+ sev_cmd_buffer_len(cmd), false);
+ return ret;
+}
+
+static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
+{
+ return -ENOTTY;
+}
+
+static const struct file_operations sev_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = sev_ioctl,
+};
+
+int sev_platform_init(struct sev_data_init *data, int *error)
+{
+ return sev_do_cmd(SEV_CMD_INIT, data, error);
+}
+EXPORT_SYMBOL_GPL(sev_platform_init);
+
+int sev_platform_shutdown(int *error)
+{
+ return sev_do_cmd(SEV_CMD_SHUTDOWN, 0, error);
+}
+EXPORT_SYMBOL_GPL(sev_platform_shutdown);
+
+int sev_platform_status(struct sev_data_status *data, int *error)
+{
+ return sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+}
+EXPORT_SYMBOL_GPL(sev_platform_status);
+
+int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
+ void *data, int *error)
+{
+ if (!filep || filep->f_op != &sev_fops)
+ return -EBADF;
+
+ return sev_do_cmd(cmd, data, error);
+}
+EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
+
+int sev_guest_deactivate(struct sev_data_deactivate *data, int *error)
+{
+ return sev_do_cmd(SEV_CMD_DEACTIVATE, data, error);
+}
+EXPORT_SYMBOL_GPL(sev_guest_deactivate);
+
+int sev_guest_activate(struct sev_data_activate *data, int *error)
+{
+ return sev_do_cmd(SEV_CMD_ACTIVATE, data, error);
+}
+EXPORT_SYMBOL_GPL(sev_guest_activate);
+
+int sev_guest_decommission(struct sev_data_decommission *data, int *error)
+{
+ return sev_do_cmd(SEV_CMD_DECOMMISSION, data, error);
+}
+EXPORT_SYMBOL_GPL(sev_guest_decommission);
+
+int sev_guest_df_flush(int *error)
+{
+ return sev_do_cmd(SEV_CMD_DF_FLUSH, 0, error);
+}
+EXPORT_SYMBOL_GPL(sev_guest_df_flush);
+
+static int sev_ops_init(struct psp_device *psp)
+{
+ struct device *dev = psp->dev;
+ struct miscdevice *misc;
+ int ret;
+
+ /*
+ * SEV feature support can be detected on multiple devices but the SEV
+ * FW commands must be issued on the master. During probe, we do not
+ * know the master hence we create /dev/sev on the first device probe.
+ * sev_do_cmd() finds the right master device to which to issue the
+ * command to the firmware.
+ */
+ if (!sev_fops_registered) {
+
+ misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL);
+ if (!misc)
+ return -ENOMEM;
+
+ misc->minor = MISC_DYNAMIC_MINOR;
+ misc->name = DEVICE_NAME;
+ misc->fops = &sev_fops;
+
+ ret = misc_register(misc);
+ if (ret)
+ return ret;
+
+ sev_fops_registered = true;
+ psp->sev_misc = misc;
+ init_waitqueue_head(&psp->sev_int_queue);
+ dev_info(dev, "registered SEV device\n");
+ }
+
+ return 0;
+}
+
+static int sev_init(struct psp_device *psp)
+{
+ /* Check if device supports SEV feature */
+ if (!(ioread32(psp->io_regs + PSP_FEATURE_REG) & 1)) {
+ dev_dbg(psp->dev, "device does not support SEV\n");
+ return 1;
+ }
+
+ return sev_ops_init(psp);
+}
+
+static void sev_exit(struct psp_device *psp)
+{
+ if (psp->sev_misc)
+ misc_deregister(psp->sev_misc);
+}
+
int psp_dev_init(struct sp_device *sp)
{
struct device *dev = sp->dev;
@@ -84,11 +328,17 @@ int psp_dev_init(struct sp_device *sp)
if (sp->set_psp_master_device)
sp->set_psp_master_device(sp);
+ ret = sev_init(psp);
+ if (ret)
+ goto e_irq;
+
/* Enable interrupt */
iowrite32(-1, psp->io_regs + PSP_P2CMSG_INTEN);
return 0;
+e_irq:
+ sp_free_psp_irq(psp->sp, psp);
e_err:
sp->psp_data = NULL;
@@ -101,5 +351,6 @@ void psp_dev_destroy(struct sp_device *sp)
{
struct psp_device *psp = sp->psp_data;
+ sev_exit(psp);
sp_free_psp_irq(sp, psp);
}
diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h
index 55b7808367c3..7a781ec20684 100644
--- a/drivers/crypto/ccp/psp-dev.h
+++ b/drivers/crypto/ccp/psp-dev.h
@@ -25,9 +25,21 @@
#include <linux/interrupt.h>
#include <linux/irqreturn.h>
#include <linux/dmaengine.h>
+#include <linux/psp-sev.h>
+#include <linux/miscdevice.h>
#include "sp-dev.h"
+#define PSP_C2PMSG(_num) ((_num) << 2)
+#define PSP_CMDRESP PSP_C2PMSG(32)
+#define PSP_CMDBUFF_ADDR_LO PSP_C2PMSG(56)
+#define PSP_CMDBUFF_ADDR_HI PSP_C2PMSG(57)
+#define PSP_FEATURE_REG PSP_C2PMSG(63)
+
+#define PSP_P2CMSG(_num) ((_num) << 2)
+#define PSP_CMD_COMPLETE_REG 1
+#define PSP_CMD_COMPLETE PSP_P2CMSG(PSP_CMD_COMPLETE_REG)
+
#define PSP_P2CMSG_INTEN 0x0110
#define PSP_P2CMSG_INTSTS 0x0114
@@ -54,6 +66,10 @@ struct psp_device {
struct sp_device *sp;
void __iomem *io_regs;
+
+ unsigned int sev_int_rcvd;
+ wait_queue_head_t sev_int_queue;
+ struct miscdevice *sev_misc;
};
#endif /* __PSP_DEV_H */
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 496375d7f6a9..5d562d49deac 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -512,4 +512,163 @@ struct sev_data_dbg {
u32 len; /* In */
} __packed;
+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
+
+/**
+ * sev_platform_init - perform SEV INIT command
+ *
+ * @init: sev_data_init structure to be processed
+ * @error: SEV command return code
+ *
+ * Returns:
+ * 0 if the SEV successfully processed the command
+ * -%ENODEV if the SEV device is not available
+ * -%ENOTSUPP if the SEV does not support SEV
+ * -%ETIMEDOUT if the SEV command timed out
+ * -%EIO if the SEV returned a non-zero return code
+ */
+int sev_platform_init(struct sev_data_init *init, int *error);
+
+/**
+ * sev_platform_shutdown - perform SEV SHUTDOWN command
+ *
+ * @error: SEV command return code
+ *
+ * Returns:
+ * 0 if the SEV successfully processed the command
+ * -%ENODEV if the SEV device is not available
+ * -%ENOTSUPP if the SEV does not support SEV
+ * -%ETIMEDOUT if the SEV command timed out
+ * -%EIO if the SEV returned a non-zero return code
+ */
+int sev_platform_shutdown(int *error);
+
+/**
+ * sev_platform_status - perform SEV PLATFORM_STATUS command
+ *
+ * @init: sev_data_status structure to be processed
+ * @error: SEV command return code
+ *
+ * Returns:
+ * 0 if the SEV successfully processed the command
+ * -%ENODEV if the SEV device is not available
+ * -%ENOTSUPP if the SEV does not support SEV
+ * -%ETIMEDOUT if the SEV command timed out
+ * -%EIO if the SEV returned a non-zero return code
+ */
+int sev_platform_status(struct sev_data_status *status, int *error);
+
+/**
+ * sev_issue_cmd_external_user - issue SEV command by other driver with a file
+ * handle.
+ *
+ * This function can be used by other drivers to issue a SEV command on
+ * behalf of userspace. The caller must pass a valid SEV file descriptor
+ * so that we know that it has access to SEV device.
+ *
+ * @filep - SEV device file pointer
+ * @cmd - command to issue
+ * @data - command buffer
+ * @error: SEV command return code
+ *
+ * Returns:
+ * 0 if the SEV successfully processed the command
+ * -%ENODEV if the SEV device is not available
+ * -%ENOTSUPP if the SEV does not support SEV
+ * -%ETIMEDOUT if the SEV command timed out
+ * -%EIO if the SEV returned a non-zero return code
+ * -%EINVAL if the SEV file descriptor is not valid
+ */
+int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
+ void *data, int *error);
+
+/**
+ * sev_guest_deactivate - perform SEV DEACTIVATE command
+ *
+ * @deactivate: sev_data_deactivate structure to be processed
+ * @sev_ret: sev command return code
+ *
+ * Returns:
+ * 0 if the sev successfully processed the command
+ * -%ENODEV if the sev device is not available
+ * -%ENOTSUPP if the sev does not support SEV
+ * -%ETIMEDOUT if the sev command timed out
+ * -%EIO if the sev returned a non-zero return code
+ */
+int sev_guest_deactivate(struct sev_data_deactivate *data, int *error);
+
+/**
+ * sev_guest_activate - perform SEV ACTIVATE command
+ *
+ * @activate: sev_data_activate structure to be processed
+ * @sev_ret: sev command return code
+ *
+ * Returns:
+ * 0 if the sev successfully processed the command
+ * -%ENODEV if the sev device is not available
+ * -%ENOTSUPP if the sev does not support SEV
+ * -%ETIMEDOUT if the sev command timed out
+ * -%EIO if the sev returned a non-zero return code
+ */
+int sev_guest_activate(struct sev_data_activate *data, int *error);
+
+/**
+ * sev_guest_df_flush - perform SEV DF_FLUSH command
+ *
+ * @sev_ret: sev command return code
+ *
+ * Returns:
+ * 0 if the sev successfully processed the command
+ * -%ENODEV if the sev device is not available
+ * -%ENOTSUPP if the sev does not support SEV
+ * -%ETIMEDOUT if the sev command timed out
+ * -%EIO if the sev returned a non-zero return code
+ */
+int sev_guest_df_flush(int *error);
+
+/**
+ * sev_guest_decommission - perform SEV DECOMMISSION command
+ *
+ * @decommission: sev_data_decommission structure to be processed
+ * @sev_ret: sev command return code
+ *
+ * Returns:
+ * 0 if the sev successfully processed the command
+ * -%ENODEV if the sev device is not available
+ * -%ENOTSUPP if the sev does not support SEV
+ * -%ETIMEDOUT if the sev command timed out
+ * -%EIO if the sev returned a non-zero return code
+ */
+int sev_guest_decommission(struct sev_data_decommission *data, int *error);
+
+#else /* !CONFIG_CRYPTO_DEV_SP_PSP */
+
+static inline int
+sev_platform_status(struct sev_data_status *status, int *error) { return -ENODEV; }
+
+static inline int
+sev_platform_init(struct sev_data_init *init, int *error) { return -ENODEV; }
+
+static inline int sev_platform_shutdown(int *error) { return -ENODEV; }
+
+static inline int
+sev_guest_deactivate(struct sev_data_deactivate *data, int *error) { return -ENODEV; }
+
+static inline int
+sev_guest_decommission(struct sev_data_decommission *data, int *error) { return -ENODEV; }
+
+static inline int
+sev_guest_activate(struct sev_data_activate *data, int *error) { return -ENODEV; }
+
+static inline int sev_guest_df_flush(int *error) { return -ENODEV; }
+
+static inline int
+sev_issue_cmd_external_user(struct file *filep,
+ unsigned int id, void *data, int *error)
+{
+ return -ENODEV;
+}
+
+#endif /* CONFIG_CRYPTO_DEV_SP_PSP */
+
#endif /* __PSP_SEV_H__ */
--
2.9.5
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-11 16:50 ` [Part2 PATCH v5.2 12.2/31] " Brijesh Singh
@ 2017-10-12 14:08 ` Borislav Petkov
2017-10-12 21:11 ` Brijesh Singh
2017-10-12 18:21 ` Borislav Petkov
1 sibling, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 14:08 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Wed, Oct 11, 2017 at 11:50:30AM -0500, Brijesh Singh wrote:
> AMD's new Secure Encrypted Virtualization (SEV) feature allows the
> memory contents of virtual machines to be transparently encrypted with a
> key unique to the VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP) which exposes the
> commands for these tasks. The complete spec is available at:
>
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
>
> Extend the AMD-SP driver to provide the following support:
>
> - an in-kernel API to communicate with the SEV firmware. The API can be
> used by the hypervisor to create encryption context for a SEV guest.
>
> - a userspace IOCTL to manage the platform certificates.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Improvements-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> Make it as a second patch in the series (changes from 12.1 -> 12.2)
>
> Changes since v5.1:
> * text streamlining (from Boris)
> * rename sev_handle_cmd -> sev_do_cmd (from Boris)
> * PSP_P2CMSG needs arg eval (from Boris)
> * use #ifdef instead of #if defined() (from Boris)
>
> drivers/crypto/ccp/psp-dev.c | 251 +++++++++++++++++++++++++++++++++++++++++++
> drivers/crypto/ccp/psp-dev.h | 16 +++
> include/linux/psp-sev.h | 159 +++++++++++++++++++++++++++
> 3 files changed, 426 insertions(+)
>
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index b5789f878560..175cb3c3b8ef 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -23,9 +23,16 @@
> #include <linux/hw_random.h>
> #include <linux/ccp.h>
>
> +#include <uapi/linux/psp-sev.h>
> +
> #include "sp-dev.h"
> #include "psp-dev.h"
>
> +#define DEVICE_NAME "sev"
> +
> +static DEFINE_MUTEX(sev_cmd_mutex);
> +static bool sev_fops_registered;
Well, if you're going to have a global var, why not pull up the misc
device instead?
And mind you, I've moved out this assignments:
+ psp->sev_misc = psp_misc_dev;
+ init_waitqueue_head(&psp->sev_int_queue);
+ dev_info(dev, "registered SEV device\n");
outside of the if-conditional as I'm assuming you want to do this for
each psp device for which sev_ops_init() is called.
Or am I wrong here?
---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 175cb3c3b8ef..d50aaa1ca75b 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -31,7 +31,7 @@
#define DEVICE_NAME "sev"
static DEFINE_MUTEX(sev_cmd_mutex);
-static bool sev_fops_registered;
+static struct miscdevice *psp_misc_dev;
static struct psp_device *psp_alloc_struct(struct sp_device *sp)
{
@@ -242,7 +242,6 @@ EXPORT_SYMBOL_GPL(sev_guest_df_flush);
static int sev_ops_init(struct psp_device *psp)
{
struct device *dev = psp->dev;
- struct miscdevice *misc;
int ret;
/*
@@ -252,26 +251,24 @@ static int sev_ops_init(struct psp_device *psp)
* sev_do_cmd() finds the right master device to which to issue the
* command to the firmware.
*/
- if (!sev_fops_registered) {
-
- misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL);
- if (!misc)
+ if (!psp_misc_dev) {
+ psp_misc_dev = devm_kzalloc(dev, sizeof(struct miscdevice), GFP_KERNEL);
+ if (!psp_misc_dev)
return -ENOMEM;
- misc->minor = MISC_DYNAMIC_MINOR;
- misc->name = DEVICE_NAME;
- misc->fops = &sev_fops;
+ psp_misc_dev->minor = MISC_DYNAMIC_MINOR;
+ psp_misc_dev->name = DEVICE_NAME;
+ psp_misc_dev->fops = &sev_fops;
- ret = misc_register(misc);
+ ret = misc_register(psp_misc_dev);
if (ret)
return ret;
-
- sev_fops_registered = true;
- psp->sev_misc = misc;
- init_waitqueue_head(&psp->sev_int_queue);
- dev_info(dev, "registered SEV device\n");
}
+ psp->sev_misc = psp_misc_dev;
+ init_waitqueue_head(&psp->sev_int_queue);
+ dev_info(dev, "registered SEV device\n");
+
return 0;
}
@@ -288,8 +285,8 @@ static int sev_init(struct psp_device *psp)
static void sev_exit(struct psp_device *psp)
{
- if (psp->sev_misc)
- misc_deregister(psp->sev_misc);
+ if (psp_misc_dev)
+ misc_deregister(psp_misc_dev);
}
int psp_dev_init(struct sp_device *sp)
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply related [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-12 14:08 ` Borislav Petkov
@ 2017-10-12 21:11 ` Brijesh Singh
2017-10-12 21:41 ` Borislav Petkov
0 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-12 21:11 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/12/17 9:08 AM, Borislav Petkov wrote:
...
> Well, if you're going to have a global var, why not pull up the misc
> device instead?
>
> And mind you, I've moved out this assignments:
>
> + psp->sev_misc = psp_misc_dev;
> + init_waitqueue_head(&psp->sev_int_queue);
> + dev_info(dev, "registered SEV device\n");
>
> outside of the if-conditional as I'm assuming you want to do this for
> each psp device for which sev_ops_init() is called.
>
> Or am I wrong here?
I am OK with your patch except one minor issue highlighted below.
> ---
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 175cb3c3b8ef..d50aaa1ca75b 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -31,7 +31,7 @@
> #define DEVICE_NAME "sev"
>
> static DEFINE_MUTEX(sev_cmd_mutex);
> -static bool sev_fops_registered;
> +static struct miscdevice *psp_misc_dev;
>
> static struct psp_device *psp_alloc_struct(struct sp_device *sp)
> {
> @@ -242,7 +242,6 @@ EXPORT_SYMBOL_GPL(sev_guest_df_flush);
> static int sev_ops_init(struct psp_device *psp)
> {
> struct device *dev = psp->dev;
> - struct miscdevice *misc;
> int ret;
>
> /*
> @@ -252,26 +251,24 @@ static int sev_ops_init(struct psp_device *psp)
> * sev_do_cmd() finds the right master device to which to issue the
> * command to the firmware.
> */
> - if (!sev_fops_registered) {
> -
> - misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL);
> - if (!misc)
> + if (!psp_misc_dev) {
> + psp_misc_dev = devm_kzalloc(dev, sizeof(struct miscdevice), GFP_KERNEL);
> + if (!psp_misc_dev)
> return -ENOMEM;
>
> - misc->minor = MISC_DYNAMIC_MINOR;
> - misc->name = DEVICE_NAME;
> - misc->fops = &sev_fops;
> + psp_misc_dev->minor = MISC_DYNAMIC_MINOR;
> + psp_misc_dev->name = DEVICE_NAME;
> + psp_misc_dev->fops = &sev_fops;
>
> - ret = misc_register(misc);
> + ret = misc_register(psp_misc_dev);
> if (ret)
> return ret;
> -
> - sev_fops_registered = true;
> - psp->sev_misc = misc;
> - init_waitqueue_head(&psp->sev_int_queue);
> - dev_info(dev, "registered SEV device\n");
> }
>
> + psp->sev_misc = psp_misc_dev;
> + init_waitqueue_head(&psp->sev_int_queue);
> + dev_info(dev, "registered SEV device\n");
> +
> return 0;
> }
>
> @@ -288,8 +285,8 @@ static int sev_init(struct psp_device *psp)
>
> static void sev_exit(struct psp_device *psp)
> {
> - if (psp->sev_misc)
> - misc_deregister(psp->sev_misc);
> + if (psp_misc_dev)
> + misc_deregister(psp_misc_dev);
The sev_exit() will be called for all the psp_device instance. we need
to set psp_misc_dev = NULL after deregistering the device.
if (psp_misc_dev) {
misc_deregister(psp_misc_dev);
psp_misc_dev = NULL;
}
> }
>
> int psp_dev_init(struct sp_device *sp)
>
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-12 21:11 ` Brijesh Singh
@ 2017-10-12 21:41 ` Borislav Petkov
2017-10-12 21:52 ` Brijesh Singh
0 siblings, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 21:41 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Thu, Oct 12, 2017 at 04:11:18PM -0500, Brijesh Singh wrote:
> The sev_exit() will be called for all the psp_device instance. we need
> to set psp_misc_dev = NULL after deregistering the device.
>
> if (psp_misc_dev) {
> misc_deregister(psp_misc_dev);
> psp_misc_dev = NULL;
Right, except we assign that misc device to every psp device:
psp->sev_misc = psp_misc_dev;
so the question now is, how do you want this to work: the misc device
should be destroyed after the last psp device is destroyed or how?
Because after the first:
psp_misc_dev = NULL;
the respective psp->sev_misc will still keep references to that device
but the device itself will be already deregistered. And that sounds
strange.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-12 21:41 ` Borislav Petkov
@ 2017-10-12 21:52 ` Brijesh Singh
2017-10-12 22:22 ` Borislav Petkov
0 siblings, 1 reply; 74+ messages in thread
From: Brijesh Singh @ 2017-10-12 21:52 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/12/17 4:41 PM, Borislav Petkov wrote:
> On Thu, Oct 12, 2017 at 04:11:18PM -0500, Brijesh Singh wrote:
>> The sev_exit() will be called for all the psp_device instance. we need
>> to set psp_misc_dev = NULL after deregistering the device.
>>
>> if (psp_misc_dev) {
>> misc_deregister(psp_misc_dev);
>> psp_misc_dev = NULL;
> Right, except we assign that misc device to every psp device:
>
> psp->sev_misc = psp_misc_dev;
>
> so the question now is, how do you want this to work: the misc device
> should be destroyed after the last psp device is destroyed or how?
We don't know when the last psp device is getting destroyed. Since we
are making the sev_misc_dev as a global variable then there is no
reason for 'sev_misc' field in the psp_device.
> Because after the first:
>
> psp_misc_dev = NULL;
>
> the respective psp->sev_misc will still keep references to that device
> but the device itself will be already deregistered. And that sounds
> strange.
See my above comment, I think the simplest solution is remove psp->sev_misc
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-12 21:52 ` Brijesh Singh
@ 2017-10-12 22:22 ` Borislav Petkov
0 siblings, 0 replies; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 22:22 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Thu, Oct 12, 2017 at 04:52:32PM -0500, Brijesh Singh wrote:
> See my above comment, I think the simplest solution is remove psp->sev_misc
Ok, so far so good.
But now you still need to track which is the last psp device and to call
misc_deregister() only when the last device exits. Because if you do
that for the first psp device as it is now, all the following devices
will see a deregistered misc device. And I don't think we want that.
You probably could do something with reference counting: Documentation/kref.txt
to track that and have the last device deregister the misc device.
Or have the "enclosing" sp-dev deregister the misc device when it is
exiting and when it is sure that there are no more psp devices...
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-11 16:50 ` [Part2 PATCH v5.2 12.2/31] " Brijesh Singh
2017-10-12 14:08 ` Borislav Petkov
@ 2017-10-12 18:21 ` Borislav Petkov
2017-10-12 20:05 ` Brijesh Singh
1 sibling, 1 reply; 74+ messages in thread
From: Borislav Petkov @ 2017-10-12 18:21 UTC (permalink / raw)
To: Brijesh Singh
Cc: Paolo Bonzini, Radim Krčmář, Herbert Xu, Gary Hook,
Tom Lendacky, linux-crypto, kvm, linux-kernel
On Wed, Oct 11, 2017 at 11:50:30AM -0500, Brijesh Singh wrote:
> +static int sev_do_cmd(int cmd, void *data, int *psp_ret)
> +{
> + unsigned int phys_lsb, phys_msb;
> + struct psp_device *psp;
> + unsigned int reg, ret;
> + struct sp_device *sp;
> +
> + sp = sp_get_psp_master_device();
> + if (!sp)
> + return -ENODEV;
> +
> + psp = sp->psp_data;
> + if (!psp)
> + return -ENODEV;
> +
> + /* Get the physical address of the command buffer */
> + phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
> + phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0;
> +
> + dev_dbg(psp->dev, "sev command id %#x buffer 0x%08x%08x\n",
> + cmd, phys_msb, phys_lsb);
> +
> + print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> + sev_cmd_buffer_len(cmd), false);
> +
> + /* Only one command at a time... */
> + mutex_lock(&sev_cmd_mutex);
> +
> + iowrite32(phys_lsb, psp->io_regs + PSP_CMDBUFF_ADDR_LO);
> + iowrite32(phys_msb, psp->io_regs + PSP_CMDBUFF_ADDR_HI);
> +
> + reg = cmd;
> + reg <<= PSP_CMDRESP_CMD_SHIFT;
> + reg |= PSP_CMDRESP_IOC;
> + iowrite32(reg, psp->io_regs + PSP_CMDRESP);
> +
> + ret = sev_wait_cmd_ioc(psp, ®);
Btw, that function returns 0 unconditionally. So you can make it return
void and...
> + if (ret)
> + goto unlock;
... remove this check and initialize ret to 0 at the beginning.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 74+ messages in thread* Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
2017-10-12 18:21 ` Borislav Petkov
@ 2017-10-12 20:05 ` Brijesh Singh
0 siblings, 0 replies; 74+ messages in thread
From: Brijesh Singh @ 2017-10-12 20:05 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, Paolo Bonzini, Radim Krčmář,
Herbert Xu, Gary Hook, Tom Lendacky, linux-crypto, kvm,
linux-kernel
On 10/12/17 1:21 PM, Borislav Petkov wrote:
.....
> Btw, that function returns 0 unconditionally. So you can make it return
> void and...
Will do
>> + if (ret)
>> + goto unlock;
> ... remove this check and initialize ret to 0 at the beginning.
>
Will do
^ permalink raw reply [flat|nested] 74+ messages in thread