Re: [RFC PATCH v2 30/32] kvm: svm: Add support for SEV DEBUG_ENCRYPT command

[Brijesh Singh]

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