From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 1/2] xen/xsm: forbid PV guest console reads Date: Mon, 30 Sep 2013 14:29:57 -0400 Message-ID: <5249C325.4070602@tycho.nsa.gov> References: <1380556122-14067-1-git-send-email-dgdegra@tycho.nsa.gov> <5249BE7802000078000F824F@nat28.tlf.novell.com> <5249B4DC.8040208@tycho.nsa.gov> <20130930180627.GD5077@phenom.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VQiET-0001F8-RS for xen-devel@lists.xenproject.org; Mon, 30 Sep 2013 18:30:10 +0000 In-Reply-To: <20130930180627.GD5077@phenom.dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: xen-devel@lists.xenproject.org, keir@xen.org, ian.campbell@citrix.com, Jan Beulich List-Id: xen-devel@lists.xenproject.org On 09/30/2013 02:06 PM, Konrad Rzeszutek Wilk wrote: > On Mon, Sep 30, 2013 at 01:29:00PM -0400, Daniel De Graaf wrote: >> On 09/30/2013 12:10 PM, Jan Beulich wrote: >>>>>> On 30.09.13 at 17:48, Daniel De Graaf wrote: >>>> When the hypervisor was compiled in debug mode (with VERBOSE defined), >>>> PV guests incorrectly had access to both read and write to the console. >>>> Change this to only allow write access; since such writes were limited >>>> by log levels in 48d50de8e0, remove the dependency on VERBOSE >>>> completely. >>> >>> I disagree, and iirc I disagreed already when you tried to drop the >>> dependency on VERBOSE with that earlier patch. >>> >>>> Reported-by: Jan Beulich >>>> Signed-off-by: Daniel De Graaf >>>> --- >>>> >>>> Alternatively, if controlling writes with VERBOSE is still desired, the >>>> ifdef VERBOSE can be retained surrounding the if() with the following >>>> commit message: >>>> >>>> >>> >>> That's what I'd want to see go in. >>> >>> Jan >> >> This patch retains the existing behavior where only HVM guests can use >> the console for output, and only via the 0xE9 I/O port. With Konrad's >> Linux patch, this means that xen_raw_console_write in non-dom0 will >> produce output only on Xen <= 4.3 (which returns -ENOSYS rather than >> -EPERM, as this code does). > > I was under the impression that what we wanted is: > - normal PV guests can do console_write > - HVM guests can do console_write > - priv guests (irregardless if they are HVM or PV) can do console_write > _and_ console_read. > > Which I think the patch below still allows right? All of your bullets are true if the hypervisor is compiled with VERBOSE (which is enabled if debug=y). Otherwise, only priv guests will be able to use console_write (and it won't matter if they are PV or HVM). Either way, only priv guests will be able to use console_read - which is the only thing the below patch actually changes. >> >> ------------------------8<---------------------------------------------- >> >> The CONSOLEIO_read operation was incorrectly allowed to PV guests if the >> hypervisor was compiled in debug mode (with VERBOSE defined). >> >> Reported-by: Jan Beulich >> Signed-off-by: Daniel De Graaf >> --- >> xen/include/xsm/dummy.h | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h >> index 2abf018..b1edd29 100644 >> --- a/xen/include/xsm/dummy.h >> +++ b/xen/include/xsm/dummy.h >> @@ -233,10 +233,10 @@ static XSM_INLINE int xsm_console_io(XSM_DEFAULT_ARG struct domain *d, int cmd) >> { >> XSM_ASSERT_ACTION(XSM_OTHER); >> #ifdef VERBOSE >> - return xsm_default_action(XSM_HOOK, current->domain, NULL); >> -#else >> - return xsm_default_action(XSM_PRIV, current->domain, NULL); >> + if ( cmd == CONSOLEIO_write ) >> + return xsm_default_action(XSM_HOOK, d, NULL); >> #endif >> + return xsm_default_action(XSM_PRIV, d, NULL); >> } >> static XSM_INLINE int xsm_profile(XSM_DEFAULT_ARG struct domain *d, int op) >> > > -- Daniel De Graaf National Security Agency