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

[Jan Beulich <jbeulich@xxxxxxxx>]

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/ ?

> __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.

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).

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

> --- 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.

Feel free to retain the R-b if you decide to make the two suggested
changes which are directly related to your change here.

Jan