Re: [PATCH 3/4] xen/version: Drop bogus return values for XENVER_platform_parameters

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 04/01/2023 4:40 pm, Jan Beulich wrote:
> On 03.01.2023 21:09, Andrew Cooper wrote:
>> A split in virtual address space is only applicable for x86 PV guests.
>> Furthermore, the information returned for x86 64bit PV guests is wrong.
>>
>> Explain the problem in version.h, stating the other information that PV 
>> guests
>> need to know.
>>
>> For 64bit PV guests, and all non-x86-PV guests, return 0, which is strictly
>> less wrong than the values currently returned.
> I disagree for the 64-bit part of this. Seeing Linux'es exposure of the
> value in sysfs I even wonder whether we can change this like you do for
> HVM. Who knows what is being inferred from the value, and by whom.

Linux's sysfs ABI isn't relevant to us here.  The sysfs ABI says it
reports what the hypervisor presents, not that it will be a nonzero number.

>> --- a/xen/include/public/version.h
>> +++ b/xen/include/public/version.h
>> @@ -42,6 +42,26 @@ typedef char xen_capabilities_info_t[1024];
>>  typedef char xen_changeset_info_t[64];
>>  #define XEN_CHANGESET_INFO_LEN (sizeof(xen_changeset_info_t))
>>  
>> +/*
>> + * This API is problematic.
>> + *
>> + * It is only applicable to guests which share pagetables with Xen (x86 PV
>> + * guests), and is supposed to identify the virtual address split between
>> + * guest kernel and Xen.
>> + *
>> + * For 32bit PV guests, it mostly does this, but the caller needs to know 
>> that
>> + * Xen lives between the split and 4G.
>> + *
>> + * For 64bit PV guests, Xen lives at the bottom of the upper canonical 
>> range.
>> + * This previously returned the start of the upper canonical range (which is
>> + * the userspace/Xen split), not the Xen/kernel split (which is 8TB further
>> + * on).  This now returns 0 because the old number wasn't correct, and
>> + * changing it to anything else would be even worse.
> Whether the guest runs user mode code in the low or high half (or in yet
> another way of splitting) isn't really dictated by the PV ABI, is it?

No, but given a choice of reporting the thing which is an architectural
boundary, or the one which is the actual split between the two adjacent
ranges, reporting the architectural boundary is clearly the unhelpful thing.

>  So
> whether the value is "wrong" is entirely unclear. Instead ...
>
>> + * For all guest types using hardware virt extentions, Xen is not mapped 
>> into
>> + * the guest kernel virtual address space.  This now return 0, where it
>> + * previously returned unrelated data.
>> + */
>>  #define XENVER_platform_parameters 5
>>  struct xen_platform_parameters {
>>      xen_ulong_t virt_start;
> ... the field name tells me that all that is being conveyed is the virtual
> address of where the hypervisor area starts.

IMO, it doesn't matter what the name of the field is.  It dates from the
days when 32bit PV was the only type of guest.

32bit PV guests really do have a variable split, so the guest kernel
really does need to get this value from Xen.

The split for 64bit PV guests is compile-time constant, hence why 64bit
PV kernels don't care.

For compat HVM, it happens to pick up the -1 from:

#ifdef CONFIG_PV32
    HYPERVISOR_COMPAT_VIRT_START(d) =
        is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
#endif

in arch_domain_create(), whereas for non-compat HVM, it gets a number in
an address space it has no connection to in the slightest.  ARM guests
end up getting XEN_VIRT_START (== 2M) handed back, but this absolutely
an internal detail that guests have no business knowing.


The only reason I'm not issuing an XSA for this is because we don't have
any pretence of KASLR in Xen.  Pretty much every other kernel gets CVEs
for infoleaks like this.

We feasibly could do KASLR in !PV builds, at which point this would
qualify for an XSA.

~Andrew