Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support

2017-09-13 Thread Brijesh Singh 



On 09/13/2017 09:17 AM, Borislav Petkov wrote:
...


+
+unlock:
+   mutex_unlock(_cmd_mutex);
+   print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
+   sev_cmd_buffer_len(cmd), false);
+   return ret;


... and here you return psp_ret == 0 even though something failed.

What I think you should do is not touch @psp_ret when you return before
the SEV command executes and *when* you return, set @psp_ret accordingly
to denote the status of the command execution.

Or if you're touching it before you execute the SEV
command and you return early, it should say something like
PSP_CMDRESP_COMMAND_DIDNT_EXECUTE or so, to tell the caller exactly what
happened.



Agreed, very good catch thank you. I will fix it.

-Brijesh

Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support

2017-09-13 Thread Borislav Petkov 
On Mon, Jul 24, 2017 at 03:02:40PM -0500, Brijesh Singh wrote:
> AMDs new Secure Encrypted Virtualization (SEV) feature allows the memory
> contents of a virtual machine to be transparently encrypted with a key
> unique to the guest 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 for various commands are
> available at:
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
> 
> This patch extends AMD-SP driver to provide:
> 
>  - a in-kernel APIs to communicate with SEV device. The APIs can be used
>by the hypervisor to create encryption context for the SEV guests.
> 
>  - a userspace IOCTL to manage the platform certificates etc
> 
> Cc: Herbert Xu 
> Cc: David S. Miller 
> Cc: Gary Hook 
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Brijesh Singh 

...

> +int sev_issue_cmd(int cmd, void *data, int *psp_ret)
> +{
> + struct sev_device *sev = get_sev_master_device();
> + unsigned int phys_lsb, phys_msb;
> + unsigned int reg, ret;
> +
> + if (!sev)
> + return -ENODEV;
> +
> + if (psp_ret)
> + *psp_ret = 0;

So you set psp_ret to 0 here...

> +
> + /* Set the physical address for the PSP */
> + phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
> + phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0;
> +
> + dev_dbg(sev->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(_cmd_mutex);
> +
> + iowrite32(phys_lsb, sev->io_regs + PSP_CMDBUFF_ADDR_LO);
> + iowrite32(phys_msb, sev->io_regs + PSP_CMDBUFF_ADDR_HI);
> + wmb();
> +
> + reg = cmd;
> + reg <<= PSP_CMDRESP_CMD_SHIFT;
> + reg |= sev_poll ? 0 : PSP_CMDRESP_IOC;
> + iowrite32(reg, sev->io_regs + PSP_CMDRESP);
> +
> + ret = sev_wait_cmd(sev, );
> + if (ret)
> + goto unlock;

... something fails here and you unlock...

> +
> + if (psp_ret)
> + *psp_ret = reg & PSP_CMDRESP_ERR_MASK;
> +
> + if (reg & PSP_CMDRESP_ERR_MASK) {
> + dev_dbg(sev->dev, "sev command %u failed (%#010x)\n",
> + cmd, reg & PSP_CMDRESP_ERR_MASK);
> + ret = -EIO;
> + }
> +
> +unlock:
> + mutex_unlock(_cmd_mutex);
> + print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> + sev_cmd_buffer_len(cmd), false);
> + return ret;

... and here you return psp_ret == 0 even though something failed.

What I think you should do is not touch @psp_ret when you return before
the SEV command executes and *when* you return, set @psp_ret accordingly
to denote the status of the command execution.

Or if you're touching it before you execute the SEV
command and you return early, it should say something like
PSP_CMDRESP_COMMAND_DIDNT_EXECUTE or so, to tell the caller exactly what
happened.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
--

Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support

2017-09-12 Thread Borislav Petkov 
On Tue, Sep 12, 2017 at 10:32:13AM -0500, Brijesh Singh wrote:
> The debug statement is very helpful during development, it gives me the full
> view of what command we send to PSP, data dump of command buffer before and
> after the request completion.  e.g when dyndbg is enabled the output looks 
> like
> this:

I'm sure it is all very helpful but you have a bunch of code which is
always built-in and useful only to developers. Which means it could
be behind #ifdef DEBUG at least and disabled on production systems.
You don't have to do it immediately but once the stuff goes up and
everything stabilizes, you could ifdef it out... Something to think
about later, I'd say.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
--

Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support

2017-09-12 Thread Brijesh Singh 

Hi Boris,

I will address all your feedback in next rev.


On 09/12/2017 09:02 AM, Borislav Petkov wrote:
...




You could make that more tabular like this:

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

which should make it more readable.

But looking at this more, this is a static mapping between the commands
and the corresponding struct sizes and you use it in

 print_hex_dump_debug("(in):  ", DUMP_PREFIX_OFFSET, 16, 2, data,
 sev_cmd_buffer_len(cmd), false);

But then, I don't see what that brings you because you're not dumping
the actual @data length but the *expected* data length based on the
command type.

And *that* you can look up in the manual and do not need it in code,
enlarging the driver unnecessarily.

...



The debug statement is very helpful during development, it gives me the full
view of what command we send to PSP, data dump of command buffer before and
after the request completion.  e.g when dyndbg is enabled the output looks like
this:

[392035.621308] ccp :02:00.2: sev command id 0x4 buffer 0x80146d232820
[392035.621312] (in):  :      
[392035.624725] (out): : 0e00   0b00  

The first debug line prints command ID, second line prints memory dump of the 
command
structure and third line prints memory dump of command structure after PSP 
processed
the command.

The caller will use sev_issue_cmd() to issue PSP command. At this time we know
the command id and a opaque pointer (this points to command structure for 
command id).
Caller does not give us length of the command structure hence we need to derive 
it
from the command id using sev_cmd_buffer_len(). The command structure length is 
fixed
for a given command id.


Thanks
Brijesh

Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support

2017-09-12 Thread Borislav Petkov 
Just a cursory review: more indepth after the redesign.

On Mon, Jul 24, 2017 at 03:02:40PM -0500, Brijesh Singh wrote:
> AMDs new Secure Encrypted Virtualization (SEV) feature allows the memory
> contents of a virtual machine to be transparently encrypted with a key
> unique to the guest 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 for various commands are

s/ for various commands are/is/

> available at:
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
> 
> This patch extends AMD-SP driver to provide:

Never say "This patch" in a commit message of a patch. It is
tautologically useless.

>  - a in-kernel APIs to communicate with SEV device. The APIs can be used

s/a/an/  with a SEV ...

>by the hypervisor to create encryption context for the SEV guests.
> 
>  - a userspace IOCTL to manage the platform certificates etc
> 
> Cc: Herbert Xu 
> Cc: David S. Miller 
> Cc: Gary Hook 
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Brijesh Singh 
> ---
>  drivers/crypto/ccp/Kconfig   |  10 +
>  drivers/crypto/ccp/Makefile  |   1 +
>  drivers/crypto/ccp/psp-dev.c |   4 +
>  drivers/crypto/ccp/psp-dev.h |  27 ++
>  drivers/crypto/ccp/sev-dev.c | 416 ++
>  drivers/crypto/ccp/sev-dev.h |  67 +
>  drivers/crypto/ccp/sev-ops.c | 457 +
>  drivers/crypto/ccp/sp-pci.c  |   2 +-
>  include/linux/psp-sev.h  | 683 
> +++
>  include/uapi/linux/psp-sev.h | 110 +++
>  10 files changed, 1776 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/crypto/ccp/sev-dev.c
>  create mode 100644 drivers/crypto/ccp/sev-dev.h
>  create mode 100644 drivers/crypto/ccp/sev-ops.c
>  create mode 100644 include/linux/psp-sev.h
>  create mode 100644 include/uapi/linux/psp-sev.h
> 
> diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
> index 41c0ff5..ae0ff1c 100644
> --- a/drivers/crypto/ccp/Kconfig
> +++ b/drivers/crypto/ccp/Kconfig
> @@ -40,3 +40,13 @@ config CRYPTO_DEV_SP_PSP
>Provide the support for AMD Platform Security Processor (PSP) device
>which can be used for communicating with Secure Encryption 
> Virtualization
>(SEV) firmware.
> +
> +config CRYPTO_DEV_PSP_SEV
> + bool "Secure Encrypted Virtualization (SEV) interface"
> + default y
> + depends on CRYPTO_DEV_CCP_DD
> + depends on CRYPTO_DEV_SP_PSP
> + help
> +  Provide the kernel and userspace (/dev/sev) interface to communicate 
> with
> +  Secure Encrypted Virtualization (SEV) firmware running inside AMD 
> Platform
> +  Security Processor (PSP)
> diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
> index 8aae4ff..94ca748 100644
> --- a/drivers/crypto/ccp/Makefile
> +++ b/drivers/crypto/ccp/Makefile
> @@ -8,6 +8,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_CCP) += ccp-dev.o \
>   ccp-debugfs.o
>  ccp-$(CONFIG_PCI) += sp-pci.o
>  ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o
> +ccp-$(CONFIG_CRYPTO_DEV_PSP_SEV) += sev-dev.o sev-ops.o
>  
>  obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o
>  ccp-crypto-objs := ccp-crypto-main.o \
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index bb0ea9a..0c9d25c 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -97,6 +97,7 @@ irqreturn_t psp_irq_handler(int irq, void *data)
>  static int psp_init(struct psp_device *psp)
>  {
>   psp_add_device(psp);
> + sev_dev_init(psp);
>  
>   return 0;
>  }
> @@ -166,17 +167,20 @@ void psp_dev_destroy(struct sp_device *sp)
>   struct psp_device *psp = sp->psp_data;
>  
>   sp_free_psp_irq(sp, psp);
> + sev_dev_destroy(psp);
>  
>   psp_del_device(psp);
>  }
>  
>  int psp_dev_resume(struct sp_device *sp)
>  {
> + sev_dev_resume(sp->psp_data);
>   return 0;
>  }
>  
>  int psp_dev_suspend(struct sp_device *sp, pm_message_t state)
>  {
> + sev_dev_suspend(sp->psp_data, state);
>   return 0;
>  }
>  
> diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h
> index 6e167b8..9334d87 100644
> --- a/drivers/crypto/ccp/psp-dev.h
> +++ b/drivers/crypto/ccp/psp-dev.h
> @@ -78,5 +78,32 @@ int psp_free_tee_irq(struct psp_device *psp, void *data);
>  struct psp_device *psp_get_master_device(void);
>  
>  extern const struct psp_vdata psp_entry;
> +#ifdef CONFIG_CRYPTO_DEV_PSP_SEV
> +
> +int sev_dev_init(struct psp_device *psp);
> +void sev_dev_destroy(struct psp_device *psp);
> +int sev_dev_resume(struct psp_device *psp);
> +int sev_dev_suspend(struct psp_device *psp, pm_message_t state);
> +
> +#else /* !CONFIG_CRYPTO_DEV_PSP_SEV */
> +
> +static inline int sev_dev_init(struct psp_device *psp)
> +{
> + return -ENODEV;
> +}