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 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?
> 
> ------------------------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)
> 

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