Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

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)
-- 

Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Brijesh Singh]

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

Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

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)
-- 

Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Brijesh Singh]

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(>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 = _fops;
> + psp_misc_dev->minor = MISC_DYNAMIC_MINOR;
> + psp_misc_dev->name = DEVICE_NAME;
> + psp_misc_dev->fops = _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(>sev_int_queue);
> - dev_info(dev, "registered SEV device\n");
>   }
>  
> + psp->sev_misc = psp_misc_dev;
> + init_waitqueue_head(>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)
>

Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

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 
> Cc: "Radim Krčmář" 
> Cc: Borislav Petkov 
> Cc: Herbert Xu 
> Cc: Gary Hook 
> Cc: Tom Lendacky 
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Improvements-by: Borislav Petkov 
> Signed-off-by: Brijesh Singh 
> ---
> 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 
>  #include 
>  
> +#include 
> +
>  #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(>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 = _fops;
+   psp_misc_dev->minor = MISC_DYNAMIC_MINOR;
+   psp_misc_dev->name = DEVICE_NAME;
+   psp_misc_dev->fops = _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(>sev_int_queue);
-   dev_info(dev, "registered SEV device\n");
}
 
+   psp->sev_misc = psp_misc_dev;
+   init_waitqueue_head(>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)
--