Re: [Xen-devel] [PATCH] x86/msr: Fix fallout from mostly c/s 832c180

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/04/2019 11:46, Jan Beulich wrote:
>
>>  The function promising not to alter the pointed-to-object
>> includes the entire child callgraph.
>>
>>
>> The code you insisted Paul to add is:
>>
>> struct vcpu *v = cv->domain->vcpu[cv->vcpu_id];
>>
>> which is identical to:
>>
>> struct vcpu *v = (struct vcpu *)cv;
> It is not identical; it is having the same effective behavior when
> compiled with today's compilers.
>
>> Which highlights very clearly that this function has undefined behaviour.
> It doesn't, no.

Yes it literally does, and even in the very first sentence you quoted.

Reproduced here:

> "If an attempt is made to modify an object defined with a const-
>  qualified type through use of an lvalue with non-const-qualified
>  type, the behavior is undefined.

There is exactly one object for this vcpu.

*cv, as defined by the prototype, is const qualified, and is this object.

*v is the same object, and mutates it.

C doesn't necessarily know that "cv->domain->vcpu[cv->vcpu_id] == cv",
but it really is an alias in practice, and therefore is UB under that rule.

>
>> An optimising compiler which uses an object, and passes a const pointer
>> to that object to a function, is permitted to retain assumptions derived
>> from that state across the function call sequence point, because the ABI
>> of the function states that the content of the object doesn't change.
> Very much not so, no. Take this simple (and granted contrived)
> example:
>
> int integer;
>
> int test(void) {
>       func(&integer);
>       return integer;
> }
>
> and in a different CU (just to avoid the effect of the compiler
> inlining the whole thing)
>
> void func(const int*pi) {
>       integer = ~*pi;
> }
>
> Various other examples are possible, including ones where
> there's nothing contrived at all.

How about a concrete example which matches the code pattern under
argument and demonstrates the issue.

void func(const int *pi)
{
    int *i = (int *)pi;

    *i = 6;
}

And in a separate translation unit.

int test(void)
{
    const int i = 4;

    func(&i);
    assert(i == 4);

    return i;
}

Funnily enough, the assert never triggers.  Even at -O0, it never gets
compiled in and test has its return value in the form `mov $4, %eax;
ret`, and the only way that occurs is because of the UB.

>
>> But if you'd prefer a different argument, how about a contradiction.
>>
>> By your interpretation, the const keyword is utterly useless because a
>> compiler must treat all const pointers as non-const, because the
>> pointed-to object can change in any arbitrary way at any point.  If this
>> were the intended interpretation, const would never have been added to
>> the C language because it would waste space in the compiler for 0 gain.
>>
>> The fact it was added demonstrates that it had real material gains,
>> which means it isn't a useless keyword, which means the compiler really
>> may depend on the content of a const pointed-to-object not changing at all.
> I doubt this, and you provide no source where you take from that
> this was the intention. And despite what you say, "const" has its
> value nevertheless - it allows the compiler to tell you when a piece
> of code modifies an object that you didn't mean to alter.
>
> Quote from the language spec:
>
> "If an attempt is made to modify an object defined with a const-
>  qualified type through use of an lvalue with non-const-qualified
>  type, the behavior is undefined. If an attempt is made to refer
>  to an object defined with a volatile-qualified type through use
>  of an lvalue with non-volatile-qualified type, the behavior is
>  undefined."
>
> And the respective footnote:
>
> "This applies to those objects that behave as if they were
>  defined with qualified types, even if they are never actually
>  defined as objects in the program (such as an object at a
>  memory-mapped input/output address)."
>
> Throughout the verb used is "defined", not "declared". If any
> struct vcpu instance actually lived in .rodata (for example),
> then (without casting away constness) it would be impossible to
> construct a non-const pointer to it. Hence there would be no
> legitimate means to create a way to modify that instance. But
> that's specifically not the case here (or in the example given).

A function which takes a const vcpu* does not know, and has no way of
proving, that the object really wasn't const.

I will admit that I made a made a mistake with the optimisation claim. 
The outer function, because it can't see the declaration of the object
itself, also can't assume there aren't other aliases.

But none of this stops the casting away of const being UB, and it still
remains completely dishonest programming to declare
vmx_set_guest_bndcfgs() as taking a const vcpu, and then modifying it.

>
>>>>  * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate
>>>>    directly.  While we expect it to be true, the result is potential type
>>>>    confusion in release builds based on several subtle aspects of the CPUID
>>>>    feature derivation logic with no other safety checks.  This also fixes 
>>>> the
>>>>    a linker error in the release build of the shim, again for !CONFIG_HVM
>>>>    reasons.
>>> I don't understand "no other safety checks": To me the "S" in
>>>
>>> XEN_CPUFEATURE(MPX,           5*32+14) /*S  Memory Protection Extensions */
>>>
>>> is clear enough. While perhaps not towards "potential type confusion"
>> "type confusion" here is mixing up v->arch.hvm and v->arch.pv, which is
>> what happens when you've actually got a PV vcpu and you call an hvm_*
>> function.
>>
>> "No other safety checks" means that cp->feat.mpx becoming accidentally
>> set results in bad things happening if a PV and HVM vcpu get mixed up.
> If whatever bit / field / variable accidentally gains a wrong value,
> bad things are going to be happening. There's no escape from this.

Agreed, but we can take active steps to limit the fallout, and this how
guest_{cpuid,rdmsr,wrmsr}() have been coded thus far.

>>> as you word it, there are other cases where we make implications
>>> from the scope stated in the public header: MSR_FLUSH_CMD, for
>>> example, is supposed to be inaccessible to PV guests, but there's no
>>> explicit !PV check in its handling code.
>> Nothing with the handling of FLUSH_CMD gets into any form of UB
>> whatsoever if cp->feat.l1d_flush becomes accidentally set for a PV guest.
> Mind me adjusting this to "Nothing ... currently gets into ..."?

Fair enough, but as this is a write-only MSR, I don't expect it to
change moving forwards.

>
>>>  I would call the current state
>>> as inconsistent (seeing e.g. guest_{rd,wr}msr_x2apic() again being
>>> behind is_hvm_domain() checks), and hence it's not really possible
>>> to derive in which case which approach is to be preferred (or, as in
>>> the case here, would be objected to).
>> The very first thing guest_{rd,wr}msr_x2apic() does is operate on
>> v->arch.hvm
> But along the lines of the previous comment: I think the checking
> done in the caller should not depend on implementation details of
> the callee. It should be consistent in itself.

I'm starting to regret removing the hvm_ prefix from these functions,
which at least makes the caller side of things more obvious.

Xen's existing style is to check before calling functions like this,
rather than for the callees to check and bail.

>
>> Anyway, as was included in the bullet point, the is_hvm_domain() check
>> is a critical part of making the shim build work, given that it depends
>> on dead code elimination.  Omitting the is_hvm_domain() check really
>> does result in a link error.
> This is one way of addressing the build problem, but not the only
> one.

But it is the one which is consistent with everywhere else in codebase.

> Yet again - I'm okay with the code changes you propose, but
> please with a more civil (and, in the case of the const aspect,
> factually correct) description.probably

Its no secret that I wrote this patch while very irritated, and the
commit message can almost certainly be phrased better, but I see nothing
which factually incorrect.

~Andrew

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