Re: [PATCH v11 21/28] sev/i386: add debug encrypt and decrypt commands

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Brijesh Singh <brijesh.singh@amd.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	kvm@vger.kernel.org, "Michael S. Tsirkin" <mst@redhat.com>,
	Stefan Hajnoczi <stefanha@gmail.com>,
	qemu-devel@nongnu.org, Alexander Graf <agraf@suse.de>,
	"Edgar E. Iglesias" <edgar.iglesias@xilinx.com>,
	Markus Armbruster <armbru@redhat.com>,
	Bruce Rogers <brogers@suse.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Marcel Apfelbaum <marcel@redhat.com>,
	Borislav Petkov <bp@suse.de>,
	Thomas Lendacky <Thomas.Lendacky@amd.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Alistair Francis <alistair.francis@xilinx.com>,
	Cornelia Huck <cornelia.huck@de.ibm.com>,
	Richard Henderson <rth@twiddle.net>,
	Peter Crosthwaite <crosthwaite.peter@gmail.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v11 21/28] sev/i386: add debug encrypt and decrypt commands
Date: Wed, 7 Mar 2018 20:11:13 +0000	[thread overview]
Message-ID: <20180307201112.GK3089@work-vm> (raw)
In-Reply-To: <7b76c3ca-dc35-8bea-2513-7b4da262c91d@amd.com>

* Brijesh Singh (brijesh.singh@amd.com) wrote:
> 
> 
> On 3/7/18 12:24 PM, Dr. David Alan Gilbert wrote:
> > * Brijesh Singh (brijesh.singh@amd.com) wrote:
> >>
> >> On 03/07/2018 11:27 AM, Dr. David Alan Gilbert wrote:
> >>
> >> [...]
> >>
> >>>> +{
> >>>> +    SEVState *s = (SEVState *)handle;
> >>>> +
> >>>> +    /* If policy does not allow debug then no need to register ops */
> >>>> +    if (s->policy & SEV_POLICY_NODBG) {
> >>>> +        return;
> >>>> +    }
> >>> So what happens if someone tries to use a gdb or monitor command when
> >>> policy didn't allow debug?  Does it end up with an obvious error
> >>> somehow?
> >>>
> >> In those cases caller will get encrypted bytes, leading to unintelligible
> >> data. It can sometime translate into  obvious errors e.g caller tries to
> >> walk guest pagtable and it gets garbage and will have trouble dumping the
> >> pgtables etc. Many times qemu calls ldphys_* functions to access the data it
> >> may get tricky to report the errors.
> > So would it make sense to have something like:
> >
> > sev_mem_cant_read(uint8_t *dst, const uint8_t *src, uint32_t len, MemTxAttrs attrs)
> > {
> >     error_report("SEV Guest policy does not allow debug access");
> >
> >     return -EPERM;
> > }
> 
> In very early patches we had something similar but I was not sure  if
> that was right thing. Any debug accesses were  printing ton of messages

OK, if it would generate silly amounts of debug then leave it as is;
but I bet it'll confuse someone in the future when they try and dig
through it for debug without realising SEV-debug is there!

Dave

> and also in some case caller actually wants to dump the memory content
> (e.g x /10gx 0x000).. what we should return in those cases ? In my
> approach was if debug was not enabled then simply don't decrypt the
> memory and provide the raw data.
> 
> There was some discussion to have very high level security policy which
> may have attribute like debug=on|off, if debug is disabled then QEMU
> monitor can display messages like debug not allowed to better inform user.
> 
> 
> > void
> > sev_set_debug_ops(void *handle, MemoryRegion *mr)
> > {
> >     SEVState *s = (SEVState *)handle;
> >
> >     /* If policy does not allow debug then no need to register ops */
> >     if (s->policy & SEV_POLICY_NODBG) {
> >         sev_ops.read = sev_mem_cant_read;
> >         sev_ops.write = sev_mem_cant_write;
> >     } else {
> >         sev_ops.read = sev_mem_read;
> >         sev_ops.write = sev_mem_write;
> >     }
> >
> > Dave
> >
> >> -Brijesh
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK