While doing minor KVM cleanup to account various kernel allocations, I
noticed that all of the SEV command buffers are allocated via kmalloc(),
even for commands whose payloads is smaller than a pointer. After much
head scratching, the only reason I could come up with for dynamically
allocating the command data is CONFIG_VMAP_STACK=y.
This series teaches __sev_do_cmd_locked() to gracefully handle vmalloc'd
command buffers by copying such buffers an internal buffer before sending
the command to the PSP. The SEV driver and KVM are then converted to use
the stack for all command buffers.
The first patch is optional, I included it in case someone wants to
backport it to stable kernels. It wouldn't actually fix bugs, but it
would make debugging issues a lot easier if they did pop up.
Tested everything except sev_ioctl_do_pek_import(), I don't know anywhere
near enough about the PSP to give it the right input.
Based on kvm/queue, commit f96be2deac9b ("KVM: x86: Support KVM VMs
sharing SEV context") to avoid a minor conflict.
Sean Christopherson (5):
crypto: ccp: Detect and reject vmalloc addresses destined for PSP
crypto: ccp: Reject SEV commands with mismatching command buffer
crypto: ccp: Play nice with vmalloc'd memory for SEV command structs
crypto: ccp: Use the stack for small SEV command buffers
KVM: SVM: Allocate SEV command structures on local stack
arch/x86/kvm/svm/sev.c | 262 +++++++++++++----------------------
drivers/crypto/ccp/sev-dev.c | 161 ++++++++++-----------
drivers/crypto/ccp/sev-dev.h | 7 +
3 files changed, 184 insertions(+), 246 deletions(-)
--
2.31.0.208.g409f899ff0-goog
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 <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Tom Lendacky <[email protected]>
Fixes: 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support")
Signed-off-by: Sean Christopherson <[email protected]>
---
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;
+
/* 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;
--
2.31.0.208.g409f899ff0-goog
WARN on and reject SEV commands that provide a valid data pointer, but do
not have a known, non-zero length. And conversely, reject commands that
take a command buffer but none is provided.
Aside from sanity checking intput, disallowing a non-null pointer without
a non-zero size will allow a future patch to cleanly handle vmalloc'd
data by copying the data to an internal __pa() friendly buffer.
Note, this also effectively prevents callers from using commands that
have a non-zero length and are not known to the kernel. This is not an
explicit goal, but arguably the side effect is a good thing from the
kernel's perspective.
Cc: Brijesh Singh <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Tom Lendacky <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
drivers/crypto/ccp/sev-dev.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 6556d220713b..4c513318f16a 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -141,6 +141,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
struct sev_device *sev;
unsigned int phys_lsb, phys_msb;
unsigned int reg, ret = 0;
+ int buf_len;
if (!psp || !psp->sev_data)
return -ENODEV;
@@ -150,7 +151,11 @@ 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)))
+ buf_len = sev_cmd_buffer_len(cmd);
+ if (WARN_ON_ONCE(!!data != !!buf_len))
+ return -EINVAL;
+
+ if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
return -EINVAL;
/* Get the physical address of the command buffer */
@@ -161,7 +166,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
cmd, phys_msb, phys_lsb, psp_timeout);
print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
- sev_cmd_buffer_len(cmd), false);
+ buf_len, false);
iowrite32(phys_lsb, sev->io_regs + sev->vdata->cmdbuff_addr_lo_reg);
iowrite32(phys_msb, sev->io_regs + sev->vdata->cmdbuff_addr_hi_reg);
@@ -197,7 +202,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
}
print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
- sev_cmd_buffer_len(cmd), false);
+ buf_len, false);
return ret;
}
--
2.31.0.208.g409f899ff0-goog
Copy vmalloc'd data to an internal buffer instead of rejecting outright
so that callers can put SEV command buffers on the stack without running
afoul of CONFIG_VMAP_STACK=y. Currently, the largest supported command
takes a 68 byte buffer, i.e. pretty much every command can be put on the
stack. Because sev_cmd_mutex is held for the entirety of a transaction,
only a single bounce buffer is required.
Use a flexible array for the buffer, sized to hold the largest known
command. Alternatively, the buffer could be a union of all known
command structs, but that would incur a higher maintenance cost due to
the need to update the union for every command in addition to updating
the existing sev_cmd_buffer_len().
Align the buffer to an 8-byte boundary, mimicking the alignment that
would be provided by the compiler if any of the structs were embedded
directly. Note, sizeof() correctly incorporates this alignment.
Cc: Brijesh Singh <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Tom Lendacky <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++++++++++++------
drivers/crypto/ccp/sev-dev.h | 7 +++++++
2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 4c513318f16a..6d5882290cfc 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -135,13 +135,14 @@ static int sev_cmd_buffer_len(int cmd)
return 0;
}
-static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
+static int __sev_do_cmd_locked(int cmd, void *__data, int *psp_ret)
{
struct psp_device *psp = psp_master;
struct sev_device *sev;
unsigned int phys_lsb, phys_msb;
unsigned int reg, ret = 0;
int buf_len;
+ void *data;
if (!psp || !psp->sev_data)
return -ENODEV;
@@ -152,11 +153,21 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
sev = psp->sev_data;
buf_len = sev_cmd_buffer_len(cmd);
- if (WARN_ON_ONCE(!!data != !!buf_len))
+ if (WARN_ON_ONCE(!!__data != !!buf_len))
return -EINVAL;
- if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
- return -EINVAL;
+ if (__data && is_vmalloc_addr(__data)) {
+ /*
+ * If the incoming buffer is virtually allocated, copy it to
+ * the driver's scratch buffer as __pa() will not work for such
+ * addresses, vmalloc_to_page() is not guaranteed to succeed,
+ * and vmalloc'd data may not be physically contiguous.
+ */
+ data = sev->cmd_buf;
+ memcpy(data, __data, buf_len);
+ } else {
+ data = __data;
+ }
/* Get the physical address of the command buffer */
phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
@@ -204,6 +215,13 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
buf_len, false);
+ /*
+ * Copy potential output from the PSP back to __data. Do this even on
+ * failure in case the caller wants to glean something from the error.
+ */
+ if (__data && data != __data)
+ memcpy(__data, data, buf_len);
+
return ret;
}
@@ -978,9 +996,12 @@ int sev_dev_init(struct psp_device *psp)
{
struct device *dev = psp->dev;
struct sev_device *sev;
- int ret = -ENOMEM;
+ int ret = -ENOMEM, cmd_buf_size = 0, i;
- sev = devm_kzalloc(dev, sizeof(*sev), GFP_KERNEL);
+ for (i = 0; i < SEV_CMD_MAX; i++)
+ cmd_buf_size = max(cmd_buf_size, sev_cmd_buffer_len(i));
+
+ sev = devm_kzalloc(dev, sizeof(*sev) + cmd_buf_size, GFP_KERNEL);
if (!sev)
goto e_err;
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index dd5c4fe82914..b43283ce2d73 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -52,6 +52,13 @@ struct sev_device {
u8 api_major;
u8 api_minor;
u8 build;
+
+ /*
+ * Buffer used for incoming commands whose physical address cannot be
+ * resolved via __pa(), e.g. stack pointers when CONFIG_VMAP_STACK=y.
+ * Note, alignment isn't strictly required.
+ */
+ u8 cmd_buf[] __aligned(8);
};
int sev_dev_init(struct psp_device *psp);
--
2.31.0.208.g409f899ff0-goog
For commands with small input/output buffers, use the local stack to
"allocate" the structures used to communicate with the PSP. Now that
__sev_do_cmd_locked() gracefully handles vmalloc'd buffers, there's no
reason to avoid using the stack, e.g. CONFIG_VMAP_STACK=y will just work.
Signed-off-by: Sean Christopherson <[email protected]>
---
drivers/crypto/ccp/sev-dev.c | 122 ++++++++++++++---------------------
1 file changed, 47 insertions(+), 75 deletions(-)
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 6d5882290cfc..6a380d483fce 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -402,7 +402,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
{
struct sev_device *sev = psp_master->sev_data;
struct sev_user_data_pek_csr input;
- struct sev_data_pek_csr *data;
+ struct sev_data_pek_csr data;
void __user *input_address;
void *blob = NULL;
int ret;
@@ -413,29 +413,24 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
/* userspace wants to query CSR length */
- if (!input.address || !input.length)
+ if (!input.address || !input.length) {
+ data.address = 0;
+ data.len = 0;
goto cmd;
+ }
/* allocate a physically contiguous buffer to store the CSR blob */
input_address = (void __user *)input.address;
- if (input.length > SEV_FW_BLOB_MAX_SIZE) {
- ret = -EFAULT;
- goto e_free;
- }
+ if (input.length > SEV_FW_BLOB_MAX_SIZE)
+ return -EFAULT;
blob = kmalloc(input.length, GFP_KERNEL);
- if (!blob) {
- ret = -ENOMEM;
- goto e_free;
- }
+ if (!blob)
+ return -ENOMEM;
- data->address = __psp_pa(blob);
- data->len = input.length;
+ data.address = __psp_pa(blob);
+ data.len = input.length;
cmd:
if (sev->state == SEV_STATE_UNINIT) {
@@ -444,10 +439,10 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
goto e_free_blob;
}
- ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, data, &argp->error);
+ ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
/* If we query the CSR length, FW responded with expected data. */
- input.length = data->len;
+ input.length = data.len;
if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
ret = -EFAULT;
@@ -461,8 +456,6 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
e_free_blob:
kfree(blob);
-e_free:
- kfree(data);
return ret;
}
@@ -594,7 +587,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
{
struct sev_device *sev = psp_master->sev_data;
struct sev_user_data_pek_cert_import input;
- struct sev_data_pek_cert_import *data;
+ struct sev_data_pek_cert_import data;
void *pek_blob, *oca_blob;
int ret;
@@ -604,19 +597,14 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
/* copy PEK certificate blobs from userspace */
pek_blob = psp_copy_user_blob(input.pek_cert_address, input.pek_cert_len);
- if (IS_ERR(pek_blob)) {
- ret = PTR_ERR(pek_blob);
- goto e_free;
- }
+ if (IS_ERR(pek_blob))
+ return PTR_ERR(pek_blob);
- data->pek_cert_address = __psp_pa(pek_blob);
- data->pek_cert_len = input.pek_cert_len;
+ data.reserved = 0;
+ data.pek_cert_address = __psp_pa(pek_blob);
+ data.pek_cert_len = input.pek_cert_len;
/* copy PEK certificate blobs from userspace */
oca_blob = psp_copy_user_blob(input.oca_cert_address, input.oca_cert_len);
@@ -625,8 +613,8 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
goto e_free_pek;
}
- data->oca_cert_address = __psp_pa(oca_blob);
- data->oca_cert_len = input.oca_cert_len;
+ data.oca_cert_address = __psp_pa(oca_blob);
+ data.oca_cert_len = input.oca_cert_len;
/* If platform is not in INIT state then transition it to INIT */
if (sev->state != SEV_STATE_INIT) {
@@ -635,21 +623,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
goto e_free_oca;
}
- ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
+ ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
e_free_oca:
kfree(oca_blob);
e_free_pek:
kfree(pek_blob);
-e_free:
- kfree(data);
return ret;
}
static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
{
struct sev_user_data_get_id2 input;
- struct sev_data_get_id *data;
+ struct sev_data_get_id data;
void __user *input_address;
void *id_blob = NULL;
int ret;
@@ -663,28 +649,25 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
input_address = (void __user *)input.address;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
if (input.address && input.length) {
id_blob = kmalloc(input.length, GFP_KERNEL);
- if (!id_blob) {
- kfree(data);
+ if (!id_blob)
return -ENOMEM;
- }
- data->address = __psp_pa(id_blob);
- data->len = input.length;
+ data.address = __psp_pa(id_blob);
+ data.len = input.length;
+ } else {
+ data.address = 0;
+ data.len = 0;
}
- ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, data, &argp->error);
+ ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, &data, &argp->error);
/*
* Firmware will return the length of the ID value (either the minimum
* required length or the actual length written), return it to the user.
*/
- input.length = data->len;
+ input.length = data.len;
if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
ret = -EFAULT;
@@ -692,7 +675,7 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
}
if (id_blob) {
- if (copy_to_user(input_address, id_blob, data->len)) {
+ if (copy_to_user(input_address, id_blob, data.len)) {
ret = -EFAULT;
goto e_free;
}
@@ -700,7 +683,6 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
e_free:
kfree(id_blob);
- kfree(data);
return ret;
}
@@ -750,7 +732,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
struct sev_device *sev = psp_master->sev_data;
struct sev_user_data_pdh_cert_export input;
void *pdh_blob = NULL, *cert_blob = NULL;
- struct sev_data_pdh_cert_export *data;
+ struct sev_data_pdh_cert_export data;
void __user *input_cert_chain_address;
void __user *input_pdh_cert_address;
int ret;
@@ -768,9 +750,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
+ memset(&data, 0, sizeof(data));
/* Userspace wants to query the certificate length. */
if (!input.pdh_cert_address ||
@@ -782,25 +762,19 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
input_cert_chain_address = (void __user *)input.cert_chain_address;
/* Allocate a physically contiguous buffer to store the PDH blob. */
- if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE) {
- ret = -EFAULT;
- goto e_free;
- }
+ if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE)
+ return -EFAULT;
/* Allocate a physically contiguous buffer to store the cert chain blob. */
- if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE) {
- ret = -EFAULT;
- goto e_free;
- }
+ if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE)
+ return -EFAULT;
pdh_blob = kmalloc(input.pdh_cert_len, GFP_KERNEL);
- if (!pdh_blob) {
- ret = -ENOMEM;
- goto e_free;
- }
+ if (!pdh_blob)
+ return -ENOMEM;
- data->pdh_cert_address = __psp_pa(pdh_blob);
- data->pdh_cert_len = input.pdh_cert_len;
+ data.pdh_cert_address = __psp_pa(pdh_blob);
+ data.pdh_cert_len = input.pdh_cert_len;
cert_blob = kmalloc(input.cert_chain_len, GFP_KERNEL);
if (!cert_blob) {
@@ -808,15 +782,15 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
goto e_free_pdh;
}
- data->cert_chain_address = __psp_pa(cert_blob);
- data->cert_chain_len = input.cert_chain_len;
+ data.cert_chain_address = __psp_pa(cert_blob);
+ data.cert_chain_len = input.cert_chain_len;
cmd:
- ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, data, &argp->error);
+ ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
/* If we query the length, FW responded with expected data. */
- input.cert_chain_len = data->cert_chain_len;
- input.pdh_cert_len = data->pdh_cert_len;
+ input.cert_chain_len = data.cert_chain_len;
+ input.pdh_cert_len = data.pdh_cert_len;
if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
ret = -EFAULT;
@@ -841,8 +815,6 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
kfree(cert_blob);
e_free_pdh:
kfree(pdh_blob);
-e_free:
- kfree(data);
return ret;
}
--
2.31.0.208.g409f899ff0-goog
Use the local stack to "allocate" the structures used to communicate with
the PSP. The largest struct used by KVM, sev_data_launch_secret, clocks
in at 52 bytes, well within the realm of reasonable stack usage. The
smallest structs are a mere 4 bytes, i.e. the pointer for the allocation
is larger than the allocation itself.
Now that the PSP driver plays nice with vmalloc pointers, putting the
data on a virtually mapped stack (CONFIG_VMAP_STACK=y) will not cause
explosions.
Cc: Brijesh Singh <[email protected]>
Cc: Tom Lendacky <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/svm/sev.c | 262 +++++++++++++++--------------------------
1 file changed, 96 insertions(+), 166 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5457138c7347..316fd39c7aef 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -150,35 +150,22 @@ static void sev_asid_free(int asid)
static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
{
- struct sev_data_decommission *decommission;
- struct sev_data_deactivate *data;
+ struct sev_data_decommission decommission;
+ struct sev_data_deactivate deactivate;
if (!handle)
return;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return;
-
- /* deactivate handle */
- data->handle = handle;
+ deactivate.handle = handle;
/* Guard DEACTIVATE against WBINVD/DF_FLUSH used in ASID recycling */
down_read(&sev_deactivate_lock);
- sev_guest_deactivate(data, NULL);
+ sev_guest_deactivate(&deactivate, NULL);
up_read(&sev_deactivate_lock);
- kfree(data);
-
- decommission = kzalloc(sizeof(*decommission), GFP_KERNEL);
- if (!decommission)
- return;
-
/* decommission handle */
- decommission->handle = handle;
- sev_guest_decommission(decommission, NULL);
-
- kfree(decommission);
+ decommission.handle = handle;
+ sev_guest_decommission(&decommission, NULL);
}
static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
@@ -216,19 +203,14 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
{
- struct sev_data_activate *data;
+ struct sev_data_activate activate;
int asid = sev_get_asid(kvm);
int ret;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- return -ENOMEM;
-
/* activate ASID on the given handle */
- data->handle = handle;
- data->asid = asid;
- ret = sev_guest_activate(data, error);
- kfree(data);
+ activate.handle = handle;
+ activate.asid = asid;
+ ret = sev_guest_activate(&activate, error);
return ret;
}
@@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
- struct sev_data_launch_start *start;
+ struct sev_data_launch_start start;
struct kvm_sev_launch_start params;
void *dh_blob, *session_blob;
int *error = &argp->error;
@@ -270,20 +252,16 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, sizeof(params)))
return -EFAULT;
- start = kzalloc(sizeof(*start), GFP_KERNEL_ACCOUNT);
- if (!start)
- return -ENOMEM;
+ memset(&start, 0, sizeof(start));
dh_blob = NULL;
if (params.dh_uaddr) {
dh_blob = psp_copy_user_blob(params.dh_uaddr, params.dh_len);
- if (IS_ERR(dh_blob)) {
- ret = PTR_ERR(dh_blob);
- goto e_free;
- }
+ if (IS_ERR(dh_blob))
+ return PTR_ERR(dh_blob);
- start->dh_cert_address = __sme_set(__pa(dh_blob));
- start->dh_cert_len = params.dh_len;
+ start.dh_cert_address = __sme_set(__pa(dh_blob));
+ start.dh_cert_len = params.dh_len;
}
session_blob = NULL;
@@ -294,40 +272,38 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
goto e_free_dh;
}
- start->session_address = __sme_set(__pa(session_blob));
- start->session_len = params.session_len;
+ start.session_address = __sme_set(__pa(session_blob));
+ start.session_len = params.session_len;
}
- start->handle = params.handle;
- start->policy = params.policy;
+ start.handle = params.handle;
+ start.policy = params.policy;
/* create memory encryption context */
- ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, start, error);
+ ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, &start, error);
if (ret)
goto e_free_session;
/* Bind ASID to this guest */
- ret = sev_bind_asid(kvm, start->handle, error);
+ ret = sev_bind_asid(kvm, start.handle, error);
if (ret)
goto e_free_session;
/* return handle to userspace */
- params.handle = start->handle;
+ params.handle = start.handle;
if (copy_to_user((void __user *)(uintptr_t)argp->data, ¶ms, sizeof(params))) {
- sev_unbind_asid(kvm, start->handle);
+ sev_unbind_asid(kvm, start.handle);
ret = -EFAULT;
goto e_free_session;
}
- sev->handle = start->handle;
+ sev->handle = start.handle;
sev->fd = argp->sev_fd;
e_free_session:
kfree(session_blob);
e_free_dh:
kfree(dh_blob);
-e_free:
- kfree(start);
return ret;
}
@@ -446,7 +422,7 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
unsigned long vaddr, vaddr_end, next_vaddr, npages, pages, size, i;
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
struct kvm_sev_launch_update_data params;
- struct sev_data_launch_update_data *data;
+ struct sev_data_launch_update_data data;
struct page **inpages;
int ret;
@@ -456,20 +432,14 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, sizeof(params)))
return -EFAULT;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- return -ENOMEM;
-
vaddr = params.uaddr;
size = params.len;
vaddr_end = vaddr + size;
/* Lock the user memory. */
inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1);
- if (IS_ERR(inpages)) {
- ret = PTR_ERR(inpages);
- goto e_free;
- }
+ if (IS_ERR(inpages))
+ return PTR_ERR(inpages);
/*
* Flush (on non-coherent CPUs) before LAUNCH_UPDATE encrypts pages in
@@ -477,6 +447,9 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
*/
sev_clflush_pages(inpages, npages);
+ data.reserved = 0;
+ data.handle = sev->handle;
+
for (i = 0; vaddr < vaddr_end; vaddr = next_vaddr, i += pages) {
int offset, len;
@@ -491,10 +464,9 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
len = min_t(size_t, ((pages * PAGE_SIZE) - offset), size);
- data->handle = sev->handle;
- data->len = len;
- data->address = __sme_page_pa(inpages[i]) + offset;
- ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, data, &argp->error);
+ data.len = len;
+ data.address = __sme_page_pa(inpages[i]) + offset;
+ ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, &data, &argp->error);
if (ret)
goto e_unpin;
@@ -510,8 +482,6 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
}
/* unlock the user pages */
sev_unpin_memory(kvm, inpages, npages);
-e_free:
- kfree(data);
return ret;
}
@@ -563,16 +533,14 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
- struct sev_data_launch_update_vmsa *vmsa;
+ struct sev_data_launch_update_vmsa vmsa;
struct kvm_vcpu *vcpu;
int i, ret;
if (!sev_es_guest(kvm))
return -ENOTTY;
- vmsa = kzalloc(sizeof(*vmsa), GFP_KERNEL);
- if (!vmsa)
- return -ENOMEM;
+ vmsa.reserved = 0;
kvm_for_each_vcpu(i, vcpu, kvm) {
struct vcpu_svm *svm = to_svm(vcpu);
@@ -580,7 +548,7 @@ static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
/* Perform some pre-encryption checks against the VMSA */
ret = sev_es_sync_vmsa(svm);
if (ret)
- goto e_free;
+ return ret;
/*
* The LAUNCH_UPDATE_VMSA command will perform in-place
@@ -590,27 +558,25 @@ static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
*/
clflush_cache_range(svm->vmsa, PAGE_SIZE);
- vmsa->handle = sev->handle;
- vmsa->address = __sme_pa(svm->vmsa);
- vmsa->len = PAGE_SIZE;
- ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, vmsa,
+ vmsa.handle = sev->handle;
+ vmsa.address = __sme_pa(svm->vmsa);
+ vmsa.len = PAGE_SIZE;
+ ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, &vmsa,
&argp->error);
if (ret)
- goto e_free;
+ return ret;
svm->vcpu.arch.guest_state_protected = true;
}
-e_free:
- kfree(vmsa);
- return ret;
+ return 0;
}
static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
void __user *measure = (void __user *)(uintptr_t)argp->data;
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
- struct sev_data_launch_measure *data;
+ struct sev_data_launch_measure data;
struct kvm_sev_launch_measure params;
void __user *p = NULL;
void *blob = NULL;
@@ -622,9 +588,7 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
if (copy_from_user(¶ms, measure, sizeof(params)))
return -EFAULT;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- return -ENOMEM;
+ memset(&data, 0, sizeof(data));
/* User wants to query the blob length */
if (!params.len)
@@ -632,23 +596,20 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
p = (void __user *)(uintptr_t)params.uaddr;
if (p) {
- if (params.len > SEV_FW_BLOB_MAX_SIZE) {
- ret = -EINVAL;
- goto e_free;
- }
+ if (params.len > SEV_FW_BLOB_MAX_SIZE)
+ return -EINVAL;
- ret = -ENOMEM;
blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
if (!blob)
- goto e_free;
+ return -ENOMEM;
- data->address = __psp_pa(blob);
- data->len = params.len;
+ data.address = __psp_pa(blob);
+ data.len = params.len;
}
cmd:
- data->handle = sev->handle;
- ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, data, &argp->error);
+ data.handle = sev->handle;
+ ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, &data, &argp->error);
/*
* If we query the session length, FW responded with expected data.
@@ -665,63 +626,50 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
}
done:
- params.len = data->len;
+ params.len = data.len;
if (copy_to_user(measure, ¶ms, sizeof(params)))
ret = -EFAULT;
e_free_blob:
kfree(blob);
-e_free:
- kfree(data);
return ret;
}
static int sev_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
- struct sev_data_launch_finish *data;
- int ret;
+ struct sev_data_launch_finish data;
if (!sev_guest(kvm))
return -ENOTTY;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- return -ENOMEM;
-
- data->handle = sev->handle;
- ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, data, &argp->error);
-
- kfree(data);
- return ret;
+ data.handle = sev->handle;
+ return sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, &data, &argp->error);
}
static int sev_guest_status(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
struct kvm_sev_guest_status params;
- struct sev_data_guest_status *data;
+ struct sev_data_guest_status data;
int ret;
if (!sev_guest(kvm))
return -ENOTTY;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- return -ENOMEM;
+ memset(&data, 0, sizeof(data));
- data->handle = sev->handle;
- ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, data, &argp->error);
+ data.handle = sev->handle;
+ ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, &data, &argp->error);
if (ret)
- goto e_free;
+ return ret;
- params.policy = data->policy;
- params.state = data->state;
- params.handle = data->handle;
+ params.policy = data.policy;
+ params.state = data.state;
+ params.handle = data.handle;
if (copy_to_user((void __user *)(uintptr_t)argp->data, ¶ms, sizeof(params)))
ret = -EFAULT;
-e_free:
- kfree(data);
+
return ret;
}
@@ -730,23 +678,17 @@ static int __sev_issue_dbg_cmd(struct kvm *kvm, unsigned long src,
int *error, bool enc)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
- struct sev_data_dbg *data;
- int ret;
+ struct sev_data_dbg data;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- return -ENOMEM;
+ data.reserved = 0;
+ data.handle = sev->handle;
+ data.dst_addr = dst;
+ data.src_addr = src;
+ data.len = size;
- data->handle = sev->handle;
- data->dst_addr = dst;
- data->src_addr = src;
- data->len = size;
-
- ret = sev_issue_cmd(kvm,
- enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,
- data, error);
- kfree(data);
- return ret;
+ return sev_issue_cmd(kvm,
+ enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,
+ &data, error);
}
static int __sev_dbg_decrypt(struct kvm *kvm, unsigned long src_paddr,
@@ -966,7 +908,7 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
- struct sev_data_launch_secret *data;
+ struct sev_data_launch_secret data;
struct kvm_sev_launch_secret params;
struct page **pages;
void *blob, *hdr;
@@ -998,41 +940,36 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
goto e_unpin_memory;
}
- ret = -ENOMEM;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- goto e_unpin_memory;
+ memset(&data, 0, sizeof(data));
offset = params.guest_uaddr & (PAGE_SIZE - 1);
- data->guest_address = __sme_page_pa(pages[0]) + offset;
- data->guest_len = params.guest_len;
+ data.guest_address = __sme_page_pa(pages[0]) + offset;
+ data.guest_len = params.guest_len;
blob = psp_copy_user_blob(params.trans_uaddr, params.trans_len);
if (IS_ERR(blob)) {
ret = PTR_ERR(blob);
- goto e_free;
+ goto e_unpin_memory;
}
- data->trans_address = __psp_pa(blob);
- data->trans_len = params.trans_len;
+ data.trans_address = __psp_pa(blob);
+ data.trans_len = params.trans_len;
hdr = psp_copy_user_blob(params.hdr_uaddr, params.hdr_len);
if (IS_ERR(hdr)) {
ret = PTR_ERR(hdr);
goto e_free_blob;
}
- data->hdr_address = __psp_pa(hdr);
- data->hdr_len = params.hdr_len;
+ data.hdr_address = __psp_pa(hdr);
+ data.hdr_len = params.hdr_len;
- data->handle = sev->handle;
- ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, data, &argp->error);
+ data.handle = sev->handle;
+ ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, &data, &argp->error);
kfree(hdr);
e_free_blob:
kfree(blob);
-e_free:
- kfree(data);
e_unpin_memory:
/* content of memory is updated, mark pages dirty */
for (i = 0; i < n; i++) {
@@ -1047,7 +984,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
void __user *report = (void __user *)(uintptr_t)argp->data;
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
- struct sev_data_attestation_report *data;
+ struct sev_data_attestation_report data;
struct kvm_sev_attestation_report params;
void __user *p;
void *blob = NULL;
@@ -1059,9 +996,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, sizeof(params)))
return -EFAULT;
- data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
- if (!data)
- return -ENOMEM;
+ memset(&data, 0, sizeof(data));
/* User wants to query the blob length */
if (!params.len)
@@ -1069,23 +1004,20 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
p = (void __user *)(uintptr_t)params.uaddr;
if (p) {
- if (params.len > SEV_FW_BLOB_MAX_SIZE) {
- ret = -EINVAL;
- goto e_free;
- }
+ if (params.len > SEV_FW_BLOB_MAX_SIZE)
+ return -EINVAL;
- ret = -ENOMEM;
blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
if (!blob)
- goto e_free;
+ return -ENOMEM;
- data->address = __psp_pa(blob);
- data->len = params.len;
- memcpy(data->mnonce, params.mnonce, sizeof(params.mnonce));
+ data.address = __psp_pa(blob);
+ data.len = params.len;
+ memcpy(data.mnonce, params.mnonce, sizeof(params.mnonce));
}
cmd:
- data->handle = sev->handle;
- ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, data, &argp->error);
+ data.handle = sev->handle;
+ ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, &data, &argp->error);
/*
* If we query the session length, FW responded with expected data.
*/
@@ -1101,13 +1033,11 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
}
done:
- params.len = data->len;
+ params.len = data.len;
if (copy_to_user(report, ¶ms, sizeof(params)))
ret = -EFAULT;
e_free_blob:
kfree(blob);
-e_free:
- kfree(data);
return ret;
}
--
2.31.0.208.g409f899ff0-goog
Le 03/04/2021 à 01:36, Sean Christopherson a écrit :
> WARN on and reject SEV commands that provide a valid data pointer, but do
> not have a known, non-zero length. And conversely, reject commands that
> take a command buffer but none is provided.
>
> Aside from sanity checking intput, disallowing a non-null pointer without
> a non-zero size will allow a future patch to cleanly handle vmalloc'd
> data by copying the data to an internal __pa() friendly buffer.
>
> Note, this also effectively prevents callers from using commands that
> have a non-zero length and are not known to the kernel. This is not an
> explicit goal, but arguably the side effect is a good thing from the
> kernel's perspective.
>
> Cc: Brijesh Singh <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> drivers/crypto/ccp/sev-dev.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 6556d220713b..4c513318f16a 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -141,6 +141,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> struct sev_device *sev;
> unsigned int phys_lsb, phys_msb;
> unsigned int reg, ret = 0;
> + int buf_len;
>
> if (!psp || !psp->sev_data)
> return -ENODEV;
> @@ -150,7 +151,11 @@ 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)))
> + buf_len = sev_cmd_buffer_len(cmd);
> + if (WARN_ON_ONCE(!!data != !!buf_len))
> + return -EINVAL;
> +
> + if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
Shouldn't it be !virt_addr_valid() instead of is_vmalloc_addr() ?
> return -EINVAL;
>
> /* Get the physical address of the command buffer */
> @@ -161,7 +166,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> cmd, phys_msb, phys_lsb, psp_timeout);
>
> print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> - sev_cmd_buffer_len(cmd), false);
> + buf_len, false);
>
> iowrite32(phys_lsb, sev->io_regs + sev->vdata->cmdbuff_addr_lo_reg);
> iowrite32(phys_msb, sev->io_regs + sev->vdata->cmdbuff_addr_hi_reg);
> @@ -197,7 +202,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> }
>
> print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> - sev_cmd_buffer_len(cmd), false);
> + buf_len, false);
>
> return ret;
> }
>
Le 03/04/2021 à 01:37, Sean Christopherson a écrit :
> Copy vmalloc'd data to an internal buffer instead of rejecting outright
> so that callers can put SEV command buffers on the stack without running
> afoul of CONFIG_VMAP_STACK=y. Currently, the largest supported command
> takes a 68 byte buffer, i.e. pretty much every command can be put on the
> stack. Because sev_cmd_mutex is held for the entirety of a transaction,
> only a single bounce buffer is required.
>
> Use a flexible array for the buffer, sized to hold the largest known
> command. Alternatively, the buffer could be a union of all known
> command structs, but that would incur a higher maintenance cost due to
> the need to update the union for every command in addition to updating
> the existing sev_cmd_buffer_len().
>
> Align the buffer to an 8-byte boundary, mimicking the alignment that
> would be provided by the compiler if any of the structs were embedded
> directly. Note, sizeof() correctly incorporates this alignment.
>
> Cc: Brijesh Singh <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++++++++++++------
> drivers/crypto/ccp/sev-dev.h | 7 +++++++
> 2 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 4c513318f16a..6d5882290cfc 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -135,13 +135,14 @@ static int sev_cmd_buffer_len(int cmd)
> return 0;
> }
>
> -static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> +static int __sev_do_cmd_locked(int cmd, void *__data, int *psp_ret)
> {
> struct psp_device *psp = psp_master;
> struct sev_device *sev;
> unsigned int phys_lsb, phys_msb;
> unsigned int reg, ret = 0;
> int buf_len;
> + void *data;
>
> if (!psp || !psp->sev_data)
> return -ENODEV;
> @@ -152,11 +153,21 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> sev = psp->sev_data;
>
> buf_len = sev_cmd_buffer_len(cmd);
> - if (WARN_ON_ONCE(!!data != !!buf_len))
> + if (WARN_ON_ONCE(!!__data != !!buf_len))
> return -EINVAL;
>
> - if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
> - return -EINVAL;
> + if (__data && is_vmalloc_addr(__data)) {
I think you want to use !virt_addr_valid() here, because not only vmalloc addresses are a problem.
For instance, module addresses are a problem as well.
> + /*
> + * If the incoming buffer is virtually allocated, copy it to
> + * the driver's scratch buffer as __pa() will not work for such
> + * addresses, vmalloc_to_page() is not guaranteed to succeed,
> + * and vmalloc'd data may not be physically contiguous.
> + */
> + data = sev->cmd_buf;
> + memcpy(data, __data, buf_len);
> + } else {
> + data = __data;
> + }
>
> /* Get the physical address of the command buffer */
> phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
> @@ -204,6 +215,13 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> buf_len, false);
>
> + /*
> + * Copy potential output from the PSP back to __data. Do this even on
> + * failure in case the caller wants to glean something from the error.
> + */
> + if (__data && data != __data)
> + memcpy(__data, data, buf_len);
> +
> return ret;
> }
>
> @@ -978,9 +996,12 @@ int sev_dev_init(struct psp_device *psp)
> {
> struct device *dev = psp->dev;
> struct sev_device *sev;
> - int ret = -ENOMEM;
> + int ret = -ENOMEM, cmd_buf_size = 0, i;
>
> - sev = devm_kzalloc(dev, sizeof(*sev), GFP_KERNEL);
> + for (i = 0; i < SEV_CMD_MAX; i++)
> + cmd_buf_size = max(cmd_buf_size, sev_cmd_buffer_len(i));
> +
> + sev = devm_kzalloc(dev, sizeof(*sev) + cmd_buf_size, GFP_KERNEL);
> if (!sev)
> goto e_err;
>
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index dd5c4fe82914..b43283ce2d73 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -52,6 +52,13 @@ struct sev_device {
> u8 api_major;
> u8 api_minor;
> u8 build;
> +
> + /*
> + * Buffer used for incoming commands whose physical address cannot be
> + * resolved via __pa(), e.g. stack pointers when CONFIG_VMAP_STACK=y.
> + * Note, alignment isn't strictly required.
> + */
> + u8 cmd_buf[] __aligned(8);
> };
>
> int sev_dev_init(struct psp_device *psp);
>
Le 03/04/2021 à 01:37, Sean Christopherson a écrit :
> Copy vmalloc'd data to an internal buffer instead of rejecting outright
> so that callers can put SEV command buffers on the stack without running
> afoul of CONFIG_VMAP_STACK=y. Currently, the largest supported command
> takes a 68 byte buffer, i.e. pretty much every command can be put on the
> stack. Because sev_cmd_mutex is held for the entirety of a transaction,
> only a single bounce buffer is required.
>
> Use a flexible array for the buffer, sized to hold the largest known
> command. Alternatively, the buffer could be a union of all known
> command structs, but that would incur a higher maintenance cost due to
> the need to update the union for every command in addition to updating
> the existing sev_cmd_buffer_len().
>
> Align the buffer to an 8-byte boundary, mimicking the alignment that
> would be provided by the compiler if any of the structs were embedded
> directly. Note, sizeof() correctly incorporates this alignment.
>
> Cc: Brijesh Singh <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++++++++++++------
> drivers/crypto/ccp/sev-dev.h | 7 +++++++
> 2 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 4c513318f16a..6d5882290cfc 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -135,13 +135,14 @@ static int sev_cmd_buffer_len(int cmd)
> return 0;
> }
>
> -static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> +static int __sev_do_cmd_locked(int cmd, void *__data, int *psp_ret)
> {
> struct psp_device *psp = psp_master;
> struct sev_device *sev;
> unsigned int phys_lsb, phys_msb;
> unsigned int reg, ret = 0;
> int buf_len;
> + void *data;
>
> if (!psp || !psp->sev_data)
> return -ENODEV;
> @@ -152,11 +153,21 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> sev = psp->sev_data;
>
> buf_len = sev_cmd_buffer_len(cmd);
> - if (WARN_ON_ONCE(!!data != !!buf_len))
> + if (WARN_ON_ONCE(!!__data != !!buf_len))
Why do you need a double !! ?
I think !__data != !buf_len should be enough.
> return -EINVAL;
>
> - if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
> - return -EINVAL;
> + if (__data && is_vmalloc_addr(__data)) {
> + /*
> + * If the incoming buffer is virtually allocated, copy it to
> + * the driver's scratch buffer as __pa() will not work for such
> + * addresses, vmalloc_to_page() is not guaranteed to succeed,
> + * and vmalloc'd data may not be physically contiguous.
> + */
> + data = sev->cmd_buf;
> + memcpy(data, __data, buf_len);
> + } else {
> + data = __data;
> + }
>
> /* Get the physical address of the command buffer */
> phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
> @@ -204,6 +215,13 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> buf_len, false);
>
> + /*
> + * Copy potential output from the PSP back to __data. Do this even on
> + * failure in case the caller wants to glean something from the error.
> + */
> + if (__data && data != __data)
IIUC, when __data is NULL, data is also NULL, so this double test is useless.
Checking data != __data should be enough
> + memcpy(__data, data, buf_len);
> +
> return ret;
> }
>
> @@ -978,9 +996,12 @@ int sev_dev_init(struct psp_device *psp)
> {
> struct device *dev = psp->dev;
> struct sev_device *sev;
> - int ret = -ENOMEM;
> + int ret = -ENOMEM, cmd_buf_size = 0, i;
>
> - sev = devm_kzalloc(dev, sizeof(*sev), GFP_KERNEL);
> + for (i = 0; i < SEV_CMD_MAX; i++)
> + cmd_buf_size = max(cmd_buf_size, sev_cmd_buffer_len(i));
> +
> + sev = devm_kzalloc(dev, sizeof(*sev) + cmd_buf_size, GFP_KERNEL);
> if (!sev)
> goto e_err;
>
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index dd5c4fe82914..b43283ce2d73 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -52,6 +52,13 @@ struct sev_device {
> u8 api_major;
> u8 api_minor;
> u8 build;
> +
> + /*
> + * Buffer used for incoming commands whose physical address cannot be
> + * resolved via __pa(), e.g. stack pointers when CONFIG_VMAP_STACK=y.
> + * Note, alignment isn't strictly required.
> + */
> + u8 cmd_buf[] __aligned(8);
> };
>
> int sev_dev_init(struct psp_device *psp);
>
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 <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Fixes: 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support")
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> 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;
>
Le 03/04/2021 à 01:37, Sean Christopherson a écrit :
> Copy vmalloc'd data to an internal buffer instead of rejecting outright
> so that callers can put SEV command buffers on the stack without running
> afoul of CONFIG_VMAP_STACK=y. Currently, the largest supported command
> takes a 68 byte buffer, i.e. pretty much every command can be put on the
> stack. Because sev_cmd_mutex is held for the entirety of a transaction,
> only a single bounce buffer is required.
>
> Use a flexible array for the buffer, sized to hold the largest known
> command. Alternatively, the buffer could be a union of all known
> command structs, but that would incur a higher maintenance cost due to
> the need to update the union for every command in addition to updating
> the existing sev_cmd_buffer_len().
>
> Align the buffer to an 8-byte boundary, mimicking the alignment that
> would be provided by the compiler if any of the structs were embedded
> directly. Note, sizeof() correctly incorporates this alignment.
>
> Cc: Brijesh Singh <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++++++++++++------
> drivers/crypto/ccp/sev-dev.h | 7 +++++++
> 2 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 4c513318f16a..6d5882290cfc 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -135,13 +135,14 @@ static int sev_cmd_buffer_len(int cmd)
> return 0;
> }
>
> -static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> +static int __sev_do_cmd_locked(int cmd, void *__data, int *psp_ret)
> {
> struct psp_device *psp = psp_master;
> struct sev_device *sev;
> unsigned int phys_lsb, phys_msb;
> unsigned int reg, ret = 0;
> int buf_len;
> + void *data;
>
> if (!psp || !psp->sev_data)
> return -ENODEV;
> @@ -152,11 +153,21 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> sev = psp->sev_data;
>
> buf_len = sev_cmd_buffer_len(cmd);
> - if (WARN_ON_ONCE(!!data != !!buf_len))
> + if (WARN_ON_ONCE(!!__data != !!buf_len))
> return -EINVAL;
>
> - if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
> - return -EINVAL;
> + if (__data && is_vmalloc_addr(__data)) {
> + /*
> + * If the incoming buffer is virtually allocated, copy it to
> + * the driver's scratch buffer as __pa() will not work for such
> + * addresses, vmalloc_to_page() is not guaranteed to succeed,
> + * and vmalloc'd data may not be physically contiguous.
> + */
> + data = sev->cmd_buf;
> + memcpy(data, __data, buf_len);
> + } else {
> + data = __data;
> + }
I don't know how big commands are, but if they are small, it would probably be more efficient to
inconditionnally copy them to the buffer rather then doing the test.
>
> /* Get the physical address of the command buffer */
> phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
> @@ -204,6 +215,13 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> buf_len, false);
>
> + /*
> + * Copy potential output from the PSP back to __data. Do this even on
> + * failure in case the caller wants to glean something from the error.
> + */
> + if (__data && data != __data)
> + memcpy(__data, data, buf_len);
> +
> return ret;
> }
>
> @@ -978,9 +996,12 @@ int sev_dev_init(struct psp_device *psp)
> {
> struct device *dev = psp->dev;
> struct sev_device *sev;
> - int ret = -ENOMEM;
> + int ret = -ENOMEM, cmd_buf_size = 0, i;
>
> - sev = devm_kzalloc(dev, sizeof(*sev), GFP_KERNEL);
> + for (i = 0; i < SEV_CMD_MAX; i++)
> + cmd_buf_size = max(cmd_buf_size, sev_cmd_buffer_len(i));
> +
> + sev = devm_kzalloc(dev, sizeof(*sev) + cmd_buf_size, GFP_KERNEL);
> if (!sev)
> goto e_err;
>
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index dd5c4fe82914..b43283ce2d73 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -52,6 +52,13 @@ struct sev_device {
> u8 api_major;
> u8 api_minor;
> u8 build;
> +
> + /*
> + * Buffer used for incoming commands whose physical address cannot be
> + * resolved via __pa(), e.g. stack pointers when CONFIG_VMAP_STACK=y.
> + * Note, alignment isn't strictly required.
> + */
> + u8 cmd_buf[] __aligned(8);
> };
>
> int sev_dev_init(struct psp_device *psp);
>
Hi Sean,
On 4/2/21 6:36 PM, Sean Christopherson wrote:
> While doing minor KVM cleanup to account various kernel allocations, I
> noticed that all of the SEV command buffers are allocated via kmalloc(),
> even for commands whose payloads is smaller than a pointer. After much
> head scratching, the only reason I could come up with for dynamically
> allocating the command data is CONFIG_VMAP_STACK=y.
>
> This series teaches __sev_do_cmd_locked() to gracefully handle vmalloc'd
> command buffers by copying such buffers an internal buffer before sending
> the command to the PSP. The SEV driver and KVM are then converted to use
> the stack for all command buffers.
Thanks for the series. Post SNP series, I was going to move all the
command buffer allocation to the stack. You are ahead of me :). I can
certainly build upon your series.
The behavior of the SEV-legacy command is changed when SNP firmware is
in the INIT state. All the legacy commands that cause a firmware to
write to memory must be in the firmware state before issuing the
command. One of my patch in the SNP series is using an internal memory
before sending the command to the PSP.
Looking forward to the SNP support, may I ask you to remove the
vmalloc'd buffer check and use a page for the internal buffer ? In SNP
series, I can simply transition the internal page to firmware state
before issuing the command.
> The first patch is optional, I included it in case someone wants to
> backport it to stable kernels. It wouldn't actually fix bugs, but it
> would make debugging issues a lot easier if they did pop up.
>
> Tested everything except sev_ioctl_do_pek_import(), I don't know anywhere
> near enough about the PSP to give it the right input.
>
> Based on kvm/queue, commit f96be2deac9b ("KVM: x86: Support KVM VMs
> sharing SEV context") to avoid a minor conflict.
>
> Sean Christopherson (5):
> crypto: ccp: Detect and reject vmalloc addresses destined for PSP
> crypto: ccp: Reject SEV commands with mismatching command buffer
> crypto: ccp: Play nice with vmalloc'd memory for SEV command structs
> crypto: ccp: Use the stack for small SEV command buffers
> KVM: SVM: Allocate SEV command structures on local stack
>
> arch/x86/kvm/svm/sev.c | 262 +++++++++++++----------------------
> drivers/crypto/ccp/sev-dev.c | 161 ++++++++++-----------
> drivers/crypto/ccp/sev-dev.h | 7 +
> 3 files changed, 184 insertions(+), 246 deletions(-)
>
On Sun, Apr 04, 2021, Christophe Leroy wrote:
>
> Le 03/04/2021 ? 01:37, Sean Christopherson a ?crit?:
> > @@ -152,11 +153,21 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> > sev = psp->sev_data;
> > buf_len = sev_cmd_buffer_len(cmd);
> > - if (WARN_ON_ONCE(!!data != !!buf_len))
> > + if (WARN_ON_ONCE(!!__data != !!buf_len))
> > return -EINVAL;
> > - if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
> > - return -EINVAL;
> > + if (__data && is_vmalloc_addr(__data)) {
> > + /*
> > + * If the incoming buffer is virtually allocated, copy it to
> > + * the driver's scratch buffer as __pa() will not work for such
> > + * addresses, vmalloc_to_page() is not guaranteed to succeed,
> > + * and vmalloc'd data may not be physically contiguous.
> > + */
> > + data = sev->cmd_buf;
> > + memcpy(data, __data, buf_len);
> > + } else {
> > + data = __data;
> > + }
>
> I don't know how big commands are, but if they are small, it would probably
> be more efficient to inconditionnally copy them to the buffer rather then
> doing the test.
Brijesh, I assume SNP support will need to copy the commands unconditionally? If
yes, it probably makes sense to do so now and avoid vmalloc dependencies
completely. And I think that would allow for the removal of status_cmd_buf and
init_cmd_buf, or is there another reason those dedicated buffers exist?
On 4/5/21 10:06 AM, Sean Christopherson wrote:
> On Sun, Apr 04, 2021, Christophe Leroy wrote:
>> Le 03/04/2021 à 01:37, Sean Christopherson a écrit :
>>> @@ -152,11 +153,21 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>>> sev = psp->sev_data;
>>> buf_len = sev_cmd_buffer_len(cmd);
>>> - if (WARN_ON_ONCE(!!data != !!buf_len))
>>> + if (WARN_ON_ONCE(!!__data != !!buf_len))
>>> return -EINVAL;
>>> - if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
>>> - return -EINVAL;
>>> + if (__data && is_vmalloc_addr(__data)) {
>>> + /*
>>> + * If the incoming buffer is virtually allocated, copy it to
>>> + * the driver's scratch buffer as __pa() will not work for such
>>> + * addresses, vmalloc_to_page() is not guaranteed to succeed,
>>> + * and vmalloc'd data may not be physically contiguous.
>>> + */
>>> + data = sev->cmd_buf;
>>> + memcpy(data, __data, buf_len);
>>> + } else {
>>> + data = __data;
>>> + }
>> I don't know how big commands are, but if they are small, it would probably
>> be more efficient to inconditionnally copy them to the buffer rather then
>> doing the test.
> Brijesh, I assume SNP support will need to copy the commands unconditionally? If
> yes, it probably makes sense to do so now and avoid vmalloc dependencies
> completely. And I think that would allow for the removal of status_cmd_buf and
> init_cmd_buf, or is there another reason those dedicated buffers exist?
Yes, we need to copy the commands unconditionally for the SNP support.
It makes sense to avoid the vmalloc dependencies. I can't think of any
reason why we would need the status_cmd_buf and init_cmd_buf after those
changes.
On 4/2/21 6:36 PM, Sean Christopherson wrote:
> WARN on and reject SEV commands that provide a valid data pointer, but do
> not have a known, non-zero length. And conversely, reject commands that
> take a command buffer but none is provided.
>
> Aside from sanity checking intput, disallowing a non-null pointer without
s/intput/input/
> a non-zero size will allow a future patch to cleanly handle vmalloc'd
> data by copying the data to an internal __pa() friendly buffer.
>
> Note, this also effectively prevents callers from using commands that
> have a non-zero length and are not known to the kernel. This is not an
> explicit goal, but arguably the side effect is a good thing from the
> kernel's perspective.
>
> Cc: Brijesh Singh <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> drivers/crypto/ccp/sev-dev.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 6556d220713b..4c513318f16a 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -141,6 +141,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> struct sev_device *sev;
> unsigned int phys_lsb, phys_msb;
> unsigned int reg, ret = 0;
> + int buf_len;
>
> if (!psp || !psp->sev_data)
> return -ENODEV;
> @@ -150,7 +151,11 @@ 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)))
> + buf_len = sev_cmd_buffer_len(cmd);
> + if (WARN_ON_ONCE(!!data != !!buf_len))
Seems a bit confusing to me. Can this just be:
if (WARN_ON_ONCE(data && !buf_len))
Or is this also trying to catch the case where buf_len is non-zero but
data is NULL?
Thanks,
Tom
> + return -EINVAL;
> +
> + if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
> return -EINVAL;
>
> /* Get the physical address of the command buffer */
> @@ -161,7 +166,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> cmd, phys_msb, phys_lsb, psp_timeout);
>
> print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> - sev_cmd_buffer_len(cmd), false);
> + buf_len, false);
>
> iowrite32(phys_lsb, sev->io_regs + sev->vdata->cmdbuff_addr_lo_reg);
> iowrite32(phys_msb, sev->io_regs + sev->vdata->cmdbuff_addr_hi_reg);
> @@ -197,7 +202,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> }
>
> print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> - sev_cmd_buffer_len(cmd), false);
> + buf_len, false);
>
> return ret;
> }
>
On Mon, Apr 05, 2021, Tom Lendacky wrote:
> On 4/2/21 6:36 PM, Sean Christopherson wrote:
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 6556d220713b..4c513318f16a 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -141,6 +141,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> > struct sev_device *sev;
> > unsigned int phys_lsb, phys_msb;
> > unsigned int reg, ret = 0;
> > + int buf_len;
> >
> > if (!psp || !psp->sev_data)
> > return -ENODEV;
> > @@ -150,7 +151,11 @@ 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)))
> > + buf_len = sev_cmd_buffer_len(cmd);
> > + if (WARN_ON_ONCE(!!data != !!buf_len))
>
> Seems a bit confusing to me. Can this just be:
>
> if (WARN_ON_ONCE(data && !buf_len))
Or as Christophe pointed out, "!data != !buf_len".
> Or is this also trying to catch the case where buf_len is non-zero but
> data is NULL?
Ya. It's not necessary to detect "buf_len && !data", but it doesn't incur
additional cost. Is there a reason _not_ to disallow that?
On 4/5/21 11:33 AM, Sean Christopherson wrote:
> On Mon, Apr 05, 2021, Tom Lendacky wrote:
>> On 4/2/21 6:36 PM, Sean Christopherson wrote:
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> index 6556d220713b..4c513318f16a 100644
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -141,6 +141,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>>> struct sev_device *sev;
>>> unsigned int phys_lsb, phys_msb;
>>> unsigned int reg, ret = 0;
>>> + int buf_len;
>>>
>>> if (!psp || !psp->sev_data)
>>> return -ENODEV;
>>> @@ -150,7 +151,11 @@ 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)))
>>> + buf_len = sev_cmd_buffer_len(cmd);
>>> + if (WARN_ON_ONCE(!!data != !!buf_len))
>>
>> Seems a bit confusing to me. Can this just be:
>>
>> if (WARN_ON_ONCE(data && !buf_len))
>
> Or as Christophe pointed out, "!data != !buf_len".
>
>> Or is this also trying to catch the case where buf_len is non-zero but
>> data is NULL?
>
> Ya. It's not necessary to detect "buf_len && !data", but it doesn't incur
> additional cost. Is there a reason _not_ to disallow that?
Nope, no reason. I was just trying to process all the not signs :)
Thanks,
Tom
>