Re: [Part2 PATCH v6.1 16/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-31 Thread Borislav Petkov 
On Mon, Oct 30, 2017 at 08:29:25PM -0500, Brijesh Singh wrote:
> Okay, Just tried static global with CONFIG_VMAP_STACK=y and I am getting
> wrong physical address with __pa. PSP command fails with error code
> "INVALID_ADDRESS". The same thing works fine with kmalloc() buffer.

Ah, right, module space is vmalloc-ed.

> To avoid repeated k*alloc calls, I could devm_kzalloc() these variable 
> during sev_init() and reuse them when needed.

Yap, something like that.

Thx.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [Part2 PATCH v6.1 16/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-30 Thread Brijesh Singh 


On 10/30/17 12:57 PM, Borislav Petkov wrote:
> On Mon, Oct 30, 2017 at 12:49:14PM -0500, Brijesh Singh wrote:
>> If the buffer is allocated on the stack then there is no guarantee that
> static global is not allocated on the stack.

Okay, Just tried static global with CONFIG_VMAP_STACK=y and I am getting
wrong physical address with __pa. PSP command fails with error code
"INVALID_ADDRESS". The same thing works fine with kmalloc() buffer.

>> I can certainly move the allocation outside, but then it may increase the
>> code size in other functions. If its not a big deal then I would prefer to
>> keep what we have.
> Avoiding repeated k*alloc calls is always a good thing. Actually kmalloc-ing 
> 20
> bytes each time sounds like it is not worth the calling overhead to me.
>

To avoid repeated k*alloc calls, I could devm_kzalloc() these variable 
during sev_init() and reuse them when needed.

Re: [Part2 PATCH v6.1 16/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-30 Thread Borislav Petkov 
On Mon, Oct 30, 2017 at 12:49:14PM -0500, Brijesh Singh wrote:
> If the buffer is allocated on the stack then there is no guarantee that

static global is not allocated on the stack.

> I can certainly move the allocation outside, but then it may increase the
> code size in other functions. If its not a big deal then I would prefer to
> keep what we have.

Avoiding repeated k*alloc calls is always a good thing. Actually kmalloc-ing 20
bytes each time sounds like it is not worth the calling overhead to me.

> The function is not used by userspace ioctl, its used by kvm drv when it
> launch/terminates the SEV guest.

Just do that directly in the ioctl instead of having a dumb function.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [Part2 PATCH v6.1 16/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-30 Thread Brijesh Singh 



On 10/30/2017 12:21 PM, Borislav Petkov wrote:
...



Useless forward declarations.



Actually its helpful in other patches. I was trying to avoid making too 
many code movement in other patches to eliminate the forward 
declarations. I guess I can fix in v7.




  static struct psp_device *psp_alloc_struct(struct sp_device *sp)
  {
struct device *dev = sp->dev;


...


+static int sev_do_cmd_locked(int cmd, void *data, int *psp_ret)


You can use the "__" prefix to denote that it is a lower-level helper:

__sev_do_cmd
__sev_do_cmd_locked

Ditto for the other locked functions.


noted


...


+static int sev_platform_init_locked(struct sev_data_init *data, int *error)
+{
+   struct psp_device *psp = psp_master;
+   struct sev_data_init *input = NULL;
+   int rc = 0;
+
+   if (!psp)
+   return -ENODEV;
+
+   if (psp->sev_state == SEV_STATE_INIT)
+   return 0;
+
+   if (!data) {
+   input = kzalloc(sizeof(*input), GFP_KERNEL);
+   if (!input)
+   return -ENOMEM;
+
+   data = input;
+   }


You can do the allocation in the enclosing function, outside of the
critical region so that you can keep it shorter.

Or even better: if you're going to synchronize the commands with a
mutex, you can define a static struct sev_data_init input in this file
which you always hand in and then you can save yourself the kmalloc
calls.



If the buffer is allocated on the stack then there is no guarantee that 
__pa() will gives us a valid physical address. IIRC, when 
CONFIG_VMAP_STACK=y then stack space is mapped similar to vmalloc'd 
storage and __pa() will not work.


Since we need to pass the physical address to PSP hence variable 
allocated on the stack will not work.


I can certainly move the allocation outside, but then it may increase 
the code size in other functions. If its not a big deal then I would 
prefer to keep what we have.



...

+
+int sev_platform_shutdown(int *error)
+{
+   if (error)
+   *error = 0;
+
+   return 0;
+}


I'm guessing that that's just bare-bones and it will get filled up in
the next patches. Otherwise it looks pretty useless.



Well, we are not expanding in other patches. I was also debating on what 
to do with this function. Since we need sev_platform_init() hence it 
made sense to add sev_platform_shutdown() as well. If we add the 
function then I wanted to make sure that we set the *error = SUCCESS so 
that caller knows that function succeeded.




If it is just to block the user from sending SHUTDOWN to the PSP
master, just do that in the ioctl directly - no need to call some empty
functions.



The function is not used by userspace ioctl, its used by kvm drv when it 
launch/terminates the SEV guest.


-Brijesh

Re: [Part2 PATCH v6.1 16/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-30 Thread Borislav Petkov 
On Sun, Oct 29, 2017 at 03:48:25PM -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:

...

> Changes since v6:
>  * Add functions to init and shutdown firmware during modprobe
>  * Add sev_state variable in psp_device to keep track of the INIT and SHUTDOWN
>state
>  * Don't allow caller to shutdown the FW because SHUTDOWN will be done during
>the module removal.
>  * Drop the fw_init_mutex and init_refcount because we no longer allow apps to
>INIT and UINIT the platform

Yes, it definitely looks much better this way. Thanks for doing that!

> 
>  drivers/crypto/ccp/psp-dev.c | 360 
> +++
>  drivers/crypto/ccp/psp-dev.h |  22 +++
>  drivers/crypto/ccp/sp-dev.c  |   9 ++
>  drivers/crypto/ccp/sp-dev.h  |   4 +
>  include/linux/psp-sev.h  | 158 +++
>  5 files changed, 553 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index b5789f878560..060f57ac08b3 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -26,6 +26,15 @@
>  #include "sp-dev.h"
>  #include "psp-dev.h"
>  
> +#define DEVICE_NAME  "sev"
> +
> +static DEFINE_MUTEX(sev_cmd_mutex);
> +static struct sev_misc_dev *misc_dev;
> +static struct psp_device *psp_master;
> +
> +static int sev_platform_shutdown_locked(int *error);
> +static int sev_platform_init_locked(struct sev_data_init *data, int *error);

Useless forward declarations.

>  static struct psp_device *psp_alloc_struct(struct sp_device *sp)
>  {
>   struct device *dev = sp->dev;

...

> +static int sev_do_cmd_locked(int cmd, void *data, int *psp_ret)

You can use the "__" prefix to denote that it is a lower-level helper:

__sev_do_cmd
__sev_do_cmd_locked

Ditto for the other locked functions.

> + struct psp_device *psp = psp_master;
> + unsigned int phys_lsb, phys_msb;
> + unsigned int reg, ret = 0;
> +
> + 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);
> +
> + 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);
> +
> + /* wait for command completion */
> + sev_wait_cmd_ioc(psp, );
> +
> + 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;
> + }
> +
> + print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> +  sev_cmd_buffer_len(cmd), false);
> +
> + return ret;
> +}

...

> +static int sev_platform_init_locked(struct sev_data_init *data, int *error)
> +{
> + struct psp_device *psp = psp_master;
> + struct sev_data_init *input = NULL;
> + int rc = 0;
> +
> + if (!psp)
> + return -ENODEV;
> +
> + if (psp->sev_state == SEV_STATE_INIT)
> + return 0;
> +
> + if (!data) {
> + input = kzalloc(sizeof(*input), GFP_KERNEL);
> + if (!input)
> + return -ENOMEM;
> +
> + data = input;
> + }

You can do the allocation in the enclosing function, outside of the
critical region so that you can keep it shorter.

Or even better: if you're going to synchronize the commands with a
mutex, you can define a static struct sev_data_init input in this file
which you always hand in and then you can save yourself the kmalloc
calls.

> +
> + rc = sev_do_cmd_locked(SEV_CMD_INIT, data, error);
> + if (rc)
> + goto e_free;
> +
> + psp->sev_state = SEV_STATE_INIT;
> + dev_dbg(psp->dev, "SEV firmware intialized\n");

WARNING: 'intialized' may be misspelled - perhaps 'initialized'?
#254: FILE: drivers/crypto/ccp/psp-dev.c:215:
+   dev_dbg(psp->dev, "SEV firmware intialized\n");

WARNING: space prohibited between function name and open parenthesis '('
#445: FILE: drivers/crypto/ccp/psp-dev.c:440:
+   data = kzalloc(sizeof (*data), GFP_KERNEL);

Ok, tell me: how many times do I

Re: [Part2 PATCH v6.1 16/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-29 Thread Brijesh Singh 
I just realized that this should be marked as "PATCH v6.1 13/38 ...". I
had some  debug patch before this hence it was pushed below in the stack.


On 10/29/17 3:48 PM, 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 
> ---
>
> Boris,
>
> I have tried to minimize the INIT -> SHUTDOWN transition by keeping state
> information in sev_state variable. Since we INIT the platform during the
> modprobe time hence we no longer need the kref count and init mutex.
> Here are list of changes.
>
> Changes since v6:
>  * Add functions to init and shutdown firmware during modprobe
>  * Add sev_state variable in psp_device to keep track of the INIT and SHUTDOWN
>state
>  * Don't allow caller to shutdown the FW because SHUTDOWN will be done during
>the module removal.
>  * Drop the fw_init_mutex and init_refcount because we no longer allow apps to
>INIT and UINIT the platform
>
>  drivers/crypto/ccp/psp-dev.c | 360 
> +++
>  drivers/crypto/ccp/psp-dev.h |  22 +++
>  drivers/crypto/ccp/sp-dev.c  |   9 ++
>  drivers/crypto/ccp/sp-dev.h  |   4 +
>  include/linux/psp-sev.h  | 158 +++
>  5 files changed, 553 insertions(+)
>
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index b5789f878560..060f57ac08b3 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -26,6 +26,15 @@
>  #include "sp-dev.h"
>  #include "psp-dev.h"
>  
> +#define DEVICE_NAME  "sev"
> +
> +static DEFINE_MUTEX(sev_cmd_mutex);
> +static struct sev_misc_dev *misc_dev;
> +static struct psp_device *psp_master;
> +
> +static int sev_platform_shutdown_locked(int *error);
> +static int sev_platform_init_locked(struct sev_data_init *data, int *error);
> +
>  static struct psp_device *psp_alloc_struct(struct sp_device *sp)
>  {
>   struct device *dev = sp->dev;
> @@ -45,9 +54,304 @@ 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(>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 void 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);
> +}
> +
> +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_user_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