Re: [Xen-devel] [PATCH 1/2] xen/xsm: forbid PV guest console reads

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Sep 30, 2013 at 02:29:57PM -0400, Daniel De Graaf wrote:
> 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 <dgdegra@xxxxxxxxxxxxx> 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 <JBeulich@xxxxxxxx>
> >>>>Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> >>>>---
> >>>>
> >>>>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.

Good! That is how it should be :-)
> 
> >>
> >>------------------------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 <JBeulich@xxxxxxxx>
> >>Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> >>---
> >>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel