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