Re: [Xen-devel] [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/01/16 09:24, Ian Campbell wrote:
> On Mon, 2016-01-18 at 18:03 +0000, Andrew Cooper wrote:
>> On 18/01/16 17:58, Roger Pau Monnà wrote:
>>> El 18/01/16 a les 11.41, Andrew Cooper ha escrit:
>>>> On 18/01/16 09:44, Jan Beulich wrote:
>>>>>>>> On 18.01.16 at 10:29, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>>> On 18/01/2016 07:43, Jan Beulich wrote:
>>>>>>>>>> On 15.01.16 at 18:45, <roger.pau@xxxxxxxxxx> wrote:
>>>>>>>> Changes since v2:
>>>>>>>>  - Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b : c
>>>>>>>> )'.
>>>>>>> Thanks, but after some more thinking about it I'm afraid there
>>>>>>> are
>>>>>>> a few more aspects to consider here:
>>>>>>>
>>>>>>>> --- a/xen/arch/x86/domain.c
>>>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>>>> @@ -542,8 +542,9 @@ int arch_domain_create(struct domain *d,
>>>>>>>> unsigned int 
>>>>>> domcr_flags,
>>>>>>>>                     d->domain_id, config->emulation_flags);
>>>>>>>>              return -EINVAL;
>>>>>>>>          }
>>>>>>>> -        if ( config->emulation_flags != 0 &&
>>>>>>>> -             (!is_hvm_domain(d) || config->emulation_flags
>>>>>>>> != XEN_X86_EMU_ALL) 
>>>>>> )
>>>>>>>> +        if ( is_hvm_domain(d) ? (config->emulation_flags !=
>>>>>>>> XEN_X86_EMU_ALL &&
>>>>>>>> +             config->emulation_flags != 0) :
>>>>>>>> +             (config->emulation_flags != XEN_X86_EMU_PIT) )
>>>>>>>>          {
>>>>>>> For one I think it would be a good idea to allow zero for PV
>>>>>>> domains,
>>>>>>> and perhaps even default new DomU-s to have the PIT flag clear.
>>>>>>> (Also - indentation.)
>>>>>>>
>>>>>>> Which gets us to the second, broader issue: These flags
>>>>>>> shouldn't
>>>>>>> be forced to a particular value during migration, but instead
>>>>>>> they
>>>>>>> should be part of the state getting migrated. Incoming domains
>>>>>>> then would - if the field is missing due to coming from an
>>>>>>> older
>>>>>>> hypervisor - have the flag default to 1.
>>>>>> There is sadly another ratsnest here.
>>>>> I've been afraid of that.
>>>>>
>>>>>> These values are needed for domain creation, which means that
>>>>>> putting
>>>>>> them anywhere in the migration stream is already too late, as the
>>>>>> domain
>>>>>> has been created before the stream header is read.
>>>>> Is that an inherent requirement, or just a result of current code
>>>>> structure?
>>>> Depends.  As far as libxc/libxl migration levels go, current code
>>>> structure.
>>>>
>>>> Whatever (eventually) gets used to set these values will however be
>>>> present in the xl configuration, which is at the very start of the
>>>> stream, and is what is used to create the new domain.
>>>>
>>>> We really don't want the libxc migrate code to be making the
>>>> DOMCTL_createdomain hypercall itself; it opens up a whole new attack
>>>> surface via cunningly-crafted save image.  The best we can do is have
>>>> a
>>>> sanity check later on.
>>>>
>>>>>  I ask because migrating the emulation flags is going to
>>>>> be a requirement for relaxing the current (almost) all-or-nothing
>>>>> policy on those flags.
>>>>>
>>>>>> In principle, the best which could occur is that a value gets
>>>>>> stashed in
>>>>>> the stream and used as a sanity check.  That will at least catch
>>>>>> the
>>>>>> case when they are different.
>>>>> That'd be a minimal first step.
>>>> This is a substantial quantity of work to do properly.  As the
>>>> emulation
>>>> flags are just one in a very long list of fields handed like this, I
>>>> don't think this issue should block the series.
>>> You certainly are more familiar with the migration code than me, but
>>> wouldn't it be enough to add a new field to libxl_domain_build_info
>>> (uint32_t emulation_flags), and teach
>>> libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_json
>>>  how to properly parse it?
>> That would let it be configured from an xl.cfg file, and would normally
>> be moved in the migration stream.  However, there is a specific option
>> in xl to restore but using a brand new configuration file.
>>
>> What it doesn't do it check that the settings for the domain in the
>> stream match the settings of the domid being restored into.
> That would be the responsibility of the user who has chosen to override the
> configuration in this way.

It is the responsibility of Xen to ensure there are no exploitable holes
due to partial or misconfiguration.

In particular, this PIT emulation patch fixes an accidental NULL pointer
dereference in Xen, due to the accidental disabling of the PIT in PV guests.

~Andrew

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