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.coop...@citrix.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
