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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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@tycho.nsa.gov> 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@suse.com>
>>>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>>> ---
>>>>
>>>> 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 <JBeulich@suse.com>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>>   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