| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 14 Sep 2017 13:08:33 +0200
From: Borislav Petkov <bp@...e.de>
To: Brijesh Singh <brijesh.singh@....com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
Thomas Gleixner <tglx@...utronix.de>,
Joerg Roedel <joro@...tes.org>,
"Michael S . Tsirkin" <mst@...hat.com>,
Paolo Bonzini <pbonzini@...hat.com>,
\"Radim Krčmář\" <rkrcmar@...hat.com>,
Tom Lendacky <thomas.lendacky@....com>
Subject: Re: [RFC Part2 PATCH v3 20/26] KVM: SVM: Add support for SEV
DEBUG_DECRYPT command
On Mon, Jul 24, 2017 at 03:02:57PM -0500, Brijesh Singh wrote:
> The command is used for decrypting a guest memory region for debug
> purposes.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
> ---
> arch/x86/kvm/svm.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 160 insertions(+)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 21f85e1..933384a 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -6058,6 +6058,162 @@ static int sev_guest_status(struct kvm *kvm, struct kvm_sev_cmd *argp)
> return ret;
> }
>
> +static int __sev_dbg_enc_dec(struct kvm *kvm, unsigned long src,
> + unsigned long dst, int size, int *error, bool enc)
> +{
> + struct sev_data_dbg *data;
on stack
> + int ret;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->handle = sev_get_handle(kvm);
> + data->dst_addr = dst;
> + data->src_addr = src;
> + data->length = size;
> +
> + ret = sev_issue_cmd(kvm,
> + enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,
> + data, error);
> + kfree(data);
> + return ret;
> +}
> +
> +/*
> + * Decrypt source memory into userspace or kernel buffer. If destination buffer
> + * or len is not aligned to 16-byte boundary then it uses intermediate buffer.
> + */
> +static int __sev_dbg_dec(struct kvm *kvm, unsigned long paddr,
> + unsigned long __user dst_uaddr,
> + unsigned long dst_kaddr, unsigned long dst_paddr,
> + int size, int *error)
> +{
> + int ret, offset, len = size;
> + struct page *tpage = NULL;
> +
> + /*
> + * Debug command works with 16-byte aligned inputs, check if all inputs
> + * (src, dst and len) are 16-byte aligned. If one of the input is not
> + * aligned then we decrypt more than requested into a temporary buffer
> + * and copy the porition of data into destination buffer.
> + */
> + if (!IS_ALIGNED(paddr, 16) || !IS_ALIGNED(dst_paddr, 16) ||
> + !IS_ALIGNED(size, 16)) {
Align vertically for better readability:
if (!IS_ALIGNED(paddr, 16) ||
!IS_ALIGNED(dst_paddr, 16) ||
!IS_ALIGNED(size, 16)) {
Also, I was going to suggest to simplify that test by OR-ing them all
but gcc does that already for ya:
.loc 1 6107 0
movq %r12, %rax # _584, tmp534
orq %rbx, %rax # _592, tmp534
testb $15, %al #, tmp534
je .L2582 #,
> + tpage = (void *)alloc_page(GFP_KERNEL);
> + if (!tpage)
> + return -ENOMEM;
> +
> + dst_paddr = __sme_page_pa(tpage);
> +
> + /*
> + * if source buffer is not aligned then offset will be used
> + * when copying the data from the temporary buffer into
> + * destination buffer.
> + */
> + offset = paddr & 15;
> +
> + /* its safe to read more than requested size. */
> + len = round_up(size + offset, 16);
> +
> + paddr = round_down(paddr, 16);
> + }
> +
> + ret = __sev_dbg_enc_dec(kvm, paddr, dst_paddr, len, error, false);
Call that function what it does: __sev_issue_dbg_cmd() or so. It is hard
to keep its name apart from the enclosing __sev_dbg_dec().
> + /*
> + * If temporary buffer is used then copy the data from temporary buffer
> + * into destination buffer.
> + */
> + if (tpage) {
> +
> + /*
> + * If destination buffer is a userspace buffer then use
> + * copy_to_user otherwise memcpy.
> + */
> + if (dst_uaddr) {
> + if (copy_to_user((uint8_t *)dst_uaddr,
> + page_address(tpage) + offset, size))
> + ret = -EFAULT;
Align arguments on the opening brace.
> + } else {
> + memcpy((void *)dst_kaddr,
> + page_address(tpage) + offset, size);
Let it stick out.
> + }
> +
> + __free_page(tpage);
> + }
> +
> + return ret;
> +}
> +
> +static int sev_dbg_decrypt(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> + unsigned long vaddr, vaddr_end, next_vaddr;
> + unsigned long dst_vaddr, dst_vaddr_end;
> + struct page **srcpage, **dstpage;
> + struct kvm_sev_dbg debug;
> + unsigned long n;
> + int ret, size;
> +
> + if (!sev_guest(kvm))
> + return -ENOTTY;
> +
> + if (copy_from_user(&debug, (void *)argp->data,
> + sizeof(struct kvm_sev_dbg)))
> + return -EFAULT;
> +
> + vaddr = debug.src_addr;
> + size = debug.length;
> + vaddr_end = vaddr + size;
> + dst_vaddr = debug.dst_addr;
> + dst_vaddr_end = dst_vaddr + size;
This allows poking in any address but I guess you want that for a
debugging use case. As you said, people will be advised to disable the
debugging feature in production systems.
> +
> + for (; vaddr < vaddr_end; vaddr = next_vaddr) {
> + int len, s_off, d_off;
> +
> + /* lock userspace source and destination page */
> + srcpage = sev_pin_memory(vaddr & PAGE_MASK, PAGE_SIZE, &n, 0);
> + if (!srcpage)
> + return -EFAULT;
> +
> + dstpage = sev_pin_memory(dst_vaddr & PAGE_MASK, PAGE_SIZE,
> + &n, 1);
Let it stick out or call those src_p and dst_p.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
Powered by blists - more mailing lists