| Message ID | 148846790758.2349.16768762953657853550.stgit@brijesh-build-machine |
|---|---|
| State | Not Applicable |
| Headers | show |
On 02/03/2017 16:18, Brijesh Singh wrote: > + data = (void *) get_zeroed_page(GFP_KERNEL); The page does not need to be zeroed, does it? > + > + if ((len & 15) || (dst_addr & 15)) { > + /* if destination address and length are not 16-byte > + * aligned then: > + * a) decrypt destination page into temporary buffer > + * b) copy source data into temporary buffer at correct offset > + * c) encrypt temporary buffer > + */ > + ret = __sev_dbg_decrypt_page(kvm, dst_addr, data, &argp->error); Ah, I see now you're using this function here for read-modify-write. data is already pinned here, so even if you keep the function it makes sense to push pinning out of __sev_dbg_decrypt_page and into sev_dbg_decrypt. > + if (ret) > + goto err_3; > + d_off = dst_addr & (PAGE_SIZE - 1); > + > + if (copy_from_user(data + d_off, > + (uint8_t *)debug.src_addr, len)) { > + ret = -EFAULT; > + goto err_3; > + } > + > + encrypt->length = PAGE_SIZE; Why decrypt/re-encrypt all the page instead of just the 16 byte area around the [dst_addr, dst_addr+len) range? > + encrypt->src_addr = __psp_pa(data); > + encrypt->dst_addr = __sev_page_pa(inpages[0]); > + } else { > + if (copy_from_user(data, (uint8_t *)debug.src_addr, len)) { > + ret = -EFAULT; > + goto err_3; > + } Do you need copy_from_user, or can you just pin/unpin memory as for DEBUG_DECRYPT? Paolo > + d_off = dst_addr & (PAGE_SIZE - 1); > + encrypt->length = len; > + encrypt->src_addr = __psp_pa(data); > + encrypt->dst_addr = __sev_page_pa(inpages[0]); > + encrypt->dst_addr += d_off; > + } > + > + encrypt->handle = sev_get_handle(kvm); > + ret = sev_issue_cmd(kvm, SEV_CMD_DBG_ENCRYPT, encrypt, &argp->error);
On 03/16/2017 06:03 AM, Paolo Bonzini wrote: > > > On 02/03/2017 16:18, Brijesh Singh wrote: >> + data = (void *) get_zeroed_page(GFP_KERNEL); > > The page does not need to be zeroed, does it? > No, we don't have to zero it. I will fix it. >> + >> + if ((len & 15) || (dst_addr & 15)) { >> + /* if destination address and length are not 16-byte >> + * aligned then: >> + * a) decrypt destination page into temporary buffer >> + * b) copy source data into temporary buffer at correct offset >> + * c) encrypt temporary buffer >> + */ >> + ret = __sev_dbg_decrypt_page(kvm, dst_addr, data, &argp->error); > > Ah, I see now you're using this function here for read-modify-write. > data is already pinned here, so even if you keep the function it makes > sense to push pinning out of __sev_dbg_decrypt_page and into > sev_dbg_decrypt. I can push out pinning part outside __sev_dbg_decrypt_page > >> + if (ret) >> + goto err_3; >> + d_off = dst_addr & (PAGE_SIZE - 1); >> + >> + if (copy_from_user(data + d_off, >> + (uint8_t *)debug.src_addr, len)) { >> + ret = -EFAULT; >> + goto err_3; >> + } >> + >> + encrypt->length = PAGE_SIZE; > > Why decrypt/re-encrypt all the page instead of just the 16 byte area > around the [dst_addr, dst_addr+len) range? > good catch, I should be fine just decrypting a 16 byte area. Will fix in next rev >> + encrypt->src_addr = __psp_pa(data); >> + encrypt->dst_addr = __sev_page_pa(inpages[0]); >> + } else { >> + if (copy_from_user(data, (uint8_t *)debug.src_addr, len)) { >> + ret = -EFAULT; >> + goto err_3; >> + } > > Do you need copy_from_user, or can you just pin/unpin memory as for > DEBUG_DECRYPT? > We can work either with pin/unpin or copy_from_user. I think I choose copy_from_user because in most of time ENCRYPT path was used when I set breakpoint through gdb which basically requires copying pretty small data into guest memory. It may be very much possible that someone can try to copy lot more data and then pin/unpin can speedup the things. -Brijesh
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index ce8819a..64899ed 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -6058,6 +6058,89 @@ static int sev_dbg_decrypt(struct kvm *kvm, struct kvm_sev_cmd *argp) return ret; } +static int sev_dbg_encrypt(struct kvm *kvm, struct kvm_sev_cmd *argp) +{ + void *data; + int len, ret, d_off; + struct page **inpages; + struct kvm_sev_dbg debug; + struct sev_data_dbg *encrypt; + unsigned long src_addr, dst_addr, npages; + + if (!sev_guest(kvm)) + return -ENOTTY; + + if (copy_from_user(&debug, argp, sizeof(*argp))) + return -EFAULT; + + if (debug.length > PAGE_SIZE) + return -EINVAL; + + len = debug.length; + src_addr = debug.src_addr; + dst_addr = debug.dst_addr; + + inpages = sev_pin_memory(dst_addr, PAGE_SIZE, &npages); + if (!inpages) + return -EFAULT; + + encrypt = kzalloc(sizeof(*encrypt), GFP_KERNEL); + if (!encrypt) { + ret = -ENOMEM; + goto err_1; + } + + data = (void *) get_zeroed_page(GFP_KERNEL); + if (!data) { + ret = -ENOMEM; + goto err_2; + } + + if ((len & 15) || (dst_addr & 15)) { + /* if destination address and length are not 16-byte + * aligned then: + * a) decrypt destination page into temporary buffer + * b) copy source data into temporary buffer at correct offset + * c) encrypt temporary buffer + */ + ret = __sev_dbg_decrypt_page(kvm, dst_addr, data, &argp->error); + if (ret) + goto err_3; + d_off = dst_addr & (PAGE_SIZE - 1); + + if (copy_from_user(data + d_off, + (uint8_t *)debug.src_addr, len)) { + ret = -EFAULT; + goto err_3; + } + + encrypt->length = PAGE_SIZE; + encrypt->src_addr = __psp_pa(data); + encrypt->dst_addr = __sev_page_pa(inpages[0]); + } else { + if (copy_from_user(data, (uint8_t *)debug.src_addr, len)) { + ret = -EFAULT; + goto err_3; + } + + d_off = dst_addr & (PAGE_SIZE - 1); + encrypt->length = len; + encrypt->src_addr = __psp_pa(data); + encrypt->dst_addr = __sev_page_pa(inpages[0]); + encrypt->dst_addr += d_off; + } + + encrypt->handle = sev_get_handle(kvm); + ret = sev_issue_cmd(kvm, SEV_CMD_DBG_ENCRYPT, encrypt, &argp->error); +err_3: + free_page((unsigned long)data); +err_2: + kfree(encrypt); +err_1: + sev_unpin_memory(inpages, npages); + return ret; +} + static int amd_memory_encryption_cmd(struct kvm *kvm, void __user *argp) { int r = -ENOTTY; @@ -6089,6 +6172,10 @@ static int amd_memory_encryption_cmd(struct kvm *kvm, void __user *argp) r = sev_dbg_decrypt(kvm, &sev_cmd); break; } + case KVM_SEV_DBG_ENCRYPT: { + r = sev_dbg_encrypt(kvm, &sev_cmd); + break; + } default: break; }
The command copies a plain text into guest memory and encrypts it using the VM encryption key. The command will be used for debug purposes (e.g setting breakpoint through gdbserver) Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/kvm/svm.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+)