Re: [PATCH v2 01/12] xen/trace: Don't over-read trace objects

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/09/2021 07:53, Jan Beulich wrote:
> On 20.09.2021 19:25, Andrew Cooper wrote:
>> In the case that 'extra' isn't a multiple of uint32_t, the calculation rounds
>> the number of bytes up, causing later logic to read unrelated bytes beyond 
>> the
>> end of the object.
>>
>> Also, asserting that the object is within TRACE_EXTRA_MAX, but truncating it
>> in release builds is rude.  Instead, reject any out-of-spec records, leaving
>> enough of a message to identify the faulty caller.
>>
>> There is one buggy race record, TRC_RTDS_BUDGET_BURN.  As it must remain
> Nit: I guess s/race/trace/ ?

Oops yes.

>
>> __packed (as cur_budget is misaligned), change bool has_extratime to uint32_t
>> to compensate.
>>
>> The new printk() can also be hit by HVMOP_xentrace, although no over-read is
>> possible.  This has no business being a hypercall in the first place, as it
>> can't be used outside of custom local debugging, so extend the uint32_t
>> requirement to HVMOP_xentrace too.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> Two remarks (plus further not directly related ones), nevertheless:
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5063,8 +5063,9 @@ long do_hvm_op(unsigned long op, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>          if ( copy_from_guest(&tr, arg, 1 ) )
>>              return -EFAULT;
>>  
>> -        if ( tr.extra_bytes > sizeof(tr.extra)
>> -             || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
>> +        if ( tr.extra_bytes % sizeof(uint32_t) ||
>> +             tr.extra_bytes > sizeof(tr.extra) ||
>> +             tr.event >> TRC_SUBCLS_SHIFT )
>>              return -EINVAL;
> Despite this being a function that supposedly no-one is to really
> use, you're breaking the interface here when really there would be a
> way to be backwards compatible: Instead of failing, pad the data to
> a 32-bit boundary. As the interface struct is large enough, this
> would look to be as simple as a memset() plus aligning extra_bytes
> upwards. Otherwise the deliberate breaking of potential existing
> callers wants making explicit in the respective paragraph of the
> description.

It is discussed, along with a justification for why an ABI change is fine.

But I could do

tr.extra_bytes = ROUNDUP(tr.extra_bytes, sizeof(uint32_t));

if you'd really prefer.


> As an aside I think at the very least the case block wants enclosing
> in "#ifdef CONFIG_TRACEBUFFER", such that builds without the support
> would indicate so to callers (albeit that indication would then be
> accompanied by a bogus log message in debug builds).

That message really needs deleting.

As a better alternative,

if ( !IS_ENABLED(CONFIG_TRACEBUFFER) )
    return -EOPNOTSUPP;

The call to __trace_var() is safe in !CONFIG_TRACEBUFFER builds.

>
> Seeing the adjacent HVMOP_get_time I also wonder who the intended
> users of that one are.

There is a very large amount of junk in hvm_op(), and to a first
approximation, I would include HVMOP_get_time in that category.

But c/s b91391491c58ac40a935e10cf0703b87d8733c38 explains why it is
necessary.  This just goes to demonstrate how broken our time handling
is.  I'll add this to the pile of things needing fixing in ABI-v2.

>
>> --- a/xen/common/sched/rt.c
>> +++ b/xen/common/sched/rt.c
>> @@ -968,18 +968,20 @@ burn_budget(const struct scheduler *ops, struct 
>> rt_unit *svc, s_time_t now)
>>      /* TRACE */
>>      {
>>          struct __packed {
>> -            unsigned unit:16, dom:16;
>> +            uint16_t unit, dom;
>>              uint64_t cur_budget;
>> -            int delta;
>> -            unsigned priority_level;
>> -            bool has_extratime;
>> -        } d;
>> -        d.dom = svc->unit->domain->domain_id;
>> -        d.unit = svc->unit->unit_id;
>> -        d.cur_budget = (uint64_t) svc->cur_budget;
>> -        d.delta = delta;
>> -        d.priority_level = svc->priority_level;
>> -        d.has_extratime = svc->flags & RTDS_extratime;
>> +            uint32_t delta;
> The original field was plain int, and aiui for a valid reason. I
> don't see why you couldn't use int32_t here.

delta can't be negative, because there is a check earlier in the function.

What is a problem is the 63=>32 bit truncation, and uint32_t here is
half as bad as int32_t.

~Andrew