Re: [Xen-devel] [PATCH V5] x86/hvm: fix domain crash when CR3 has the noflush bit set

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 02/27/2018 06:26 PM, George Dunlap wrote:
>>> As an aside -- are you sure clearing the NOFLUSH from reported CR3
>>> values during introspection is the right thing to do?  You don't think
>>> your introspection engine will ever want to know if the guest OS is
>>> setting this bit?
>>
>> We can't be sure this will never be useful to know, but at least for now
>> I've not seen any requests to be able to, and our introspection engine
>> is not interested in the information (in fact, one of the reasons why
>> we've even missed the problem until it's been reported is that we
>> haven't even been subscribing to CR3 write events for a while now).
>>
>> So as far as we're concerned, losing the information about the NOFLUSH
>> bit is no problem at all (and it's definitely preferable to a domain
>> crash). Since Tamas has acked the patch, it's safe to assume that they
>> have no problem with it either, and Bitweasil seemed happy with the
>> solution as well.
>>
>> I suppose we can always write a patch later if it turns out that this is
>> valuable information.
> 
> Well if you want to maintain backwards compatibility, you'd need to
> either have the bit opt-in, or pass the noflush bit back somewhere else
> (either with a flag or with a different part of the struct).
> 
> If everyone is happy with it I don't mind.

A bool-like .noflush field could be nice, except it would only apply to
CR3 but affect both the common CR structure:

243 struct vm_event_write_ctrlreg {
244     uint32_t index;
245     uint32_t _pad;
246     uint64_t new_value;
247     uint64_t old_value;
248 };

and the common CR monitor function:

 33 bool hvm_monitor_cr(unsigned int index, unsigned long value,
unsigned long old)
 34 {
 35     struct vcpu *curr = current;
 36     struct arch_domain *ad = &curr->domain->arch;
 37     unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
 38
 39     if ( index == VM_EVENT_X86_CR3 && hvm_pcid_enabled(curr) )
 40         value &= ~X86_CR3_NOFLUSH; /* Clear the noflush bit. */
 41
 42     if ( (ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) &&
 43          (!(ad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
 44           value != old) &&
 45          ((value ^ old) & ~ad->monitor.write_ctrlreg_mask[index]) )
 46     {
 47         bool sync = ad->monitor.write_ctrlreg_sync & ctrlreg_bitmask;
 48
 49         vm_event_request_t req = {
 50             .reason = VM_EVENT_REASON_WRITE_CTRLREG,
 51             .u.write_ctrlreg.index = index,
 52             .u.write_ctrlreg.new_value = value,
 53             .u.write_ctrlreg.old_value = old
 54         };
 55
 56         if ( monitor_traps(curr, sync, &req) >= 0 )
 57             return 1;
 58     }
 59
 60     return 0;
 61 }

There's the additional problem of old vs. new values, as compared in the
above code: the way things work, the previous (old) value will always be
NOFLUSH-free (since we clear the NOFLUSH bit before storing). That means
that a NOFLUSH-set new value, identical to the old one except for the
NOFLUSH bit will trigger an "onchangeonly" event - which we don't want,
since the actual values are really identical (it's just that the NOFLUSH
bit has been removed before storing previously).

For example: previously we had (0x1 | NOFLUSH). We didn't flush, and
we've cleared the NOFLUSH bit, and now old value == 0x1.

CR3 is now being set again to (0x1 | NOFLUSH). Compared to the old
value, it's different, but is it? Was the previous value simply 0x1? Or
was it (0x1 | NOFLUSH)?


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel