[1/5] crypto: ccp: Detect and reject vmalloc addresses destined for PSP
diff mbox series

Message ID 20210402233702.3291792-2-seanjc@google.com
State New, archived
Headers show
Series
  • ccp: KVM: SVM: Use stack for SEV command buffers
Related show

Commit Message

Sean Christopherson April 2, 2021, 11:36 p.m. UTC
Explicitly reject vmalloc'd data as the source for SEV commands that are
sent to the PSP.  The PSP works with physical addresses, and __pa() will
not return the correct address for a vmalloc'd pionter, which at best
will cause the command to fail, and at worst lead to system instability.

While it's unlikely that callers will deliberately use vmalloc() for SEV
buffers, a caller can easily use a vmalloc'd pointer unknowingly when
running with CONFIG_VMAP_STACK=y as it's not obvious that putting the
command buffers on the stack would be bad.  The command buffers are
relative small and easily fit on the stack, and the APIs to do not
document that the incoming pointer must be a physically contiguous,
__pa() friendly pointer.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Fixes: 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Christophe Leroy April 4, 2021, 6:31 a.m. UTC | #1
Le 03/04/2021 à 01:36, Sean Christopherson a écrit :
> Explicitly reject vmalloc'd data as the source for SEV commands that are
> sent to the PSP.  The PSP works with physical addresses, and __pa() will
> not return the correct address for a vmalloc'd pionter, which at best
> will cause the command to fail, and at worst lead to system instability.
> 
> While it's unlikely that callers will deliberately use vmalloc() for SEV
> buffers, a caller can easily use a vmalloc'd pointer unknowingly when
> running with CONFIG_VMAP_STACK=y as it's not obvious that putting the
> command buffers on the stack would be bad.  The command buffers are
> relative small and easily fit on the stack, and the APIs to do not
> document that the incoming pointer must be a physically contiguous,
> __pa() friendly pointer.
> 
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Fixes: 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   drivers/crypto/ccp/sev-dev.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index cb9b4c4e371e..6556d220713b 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -150,6 +150,9 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>   
>   	sev = psp->sev_data;
>   
> +	if (data && WARN_ON_ONCE(is_vmalloc_addr(data)))
> +		return -EINVAL;
> +

I hadn't seen this patch when I commented the 2 other ones, I received it only this night.

As commented in the other patches, is_vmalloc_addr() is not the best way to test that __pa() can be 
safely used.

For that, you have virt_addr_valid()

>   	/* 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;
>

Patch
diff mbox series

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index cb9b4c4e371e..6556d220713b 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -150,6 +150,9 @@  static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 
 	sev = psp->sev_data;
 
+	if (data && WARN_ON_ONCE(is_vmalloc_addr(data)))
+		return -EINVAL;
+
 	/* 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;