On 17/09/2021 13:58, Jan Beulich wrote: > On 17.09.2021 10:45, Andrew Cooper wrote: >> --- a/xen/common/trace.c >> +++ b/xen/common/trace.c >> @@ -686,22 +686,21 @@ void __trace_var(u32 event, bool_t cycles, unsigned >> int extra, >> unsigned long flags; >> u32 bytes_to_tail, bytes_to_wrap; >> unsigned int rec_size, total_size; >> - unsigned int extra_word; >> bool_t started_below_highwater; >> >> if( !tb_init_done ) >> return; >> >> - /* Convert byte count into word count, rounding up */ >> - extra_word = (extra / sizeof(u32)); >> - if ( (extra % sizeof(u32)) != 0 ) >> - extra_word++; >> - >> - ASSERT(extra_word <= TRACE_EXTRA_MAX); >> - extra_word = min_t(int, extra_word, TRACE_EXTRA_MAX); >> - >> - /* Round size up to nearest word */ >> - extra = extra_word * sizeof(u32); >> + /* >> + * Trace records require extra data which is an exact multiple of >> + * uint32_t. Reject out-of-spec records. Any failure here is an error >> in >> + * the caller. >> + */ > Hmm, is "require" accurate?
In terms of "what will go wrong if this condition is violated", yes. > They may very well come without extra data > afaics. 0 is fine, and used by plenty of records, and also permitted by the filtering logic. > >> + if ( extra % sizeof(uint32_t) || >> + extra / sizeof(uint32_t) > TRACE_EXTRA_MAX ) >> + return printk_once(XENLOG_WARNING >> + "Trace event %#x bad size %u, discarding\n", >> + event, extra); > Any HVM guest looks to be able to trivially trigger this log message > (via HVMOP_xentrace), thus pointing out an issue in a guest and hiding > any other Xen related output. I'd like to suggest to adjust that call > site in prereq patch (I'm not overly fussed which of the two relatively > obvious ways). > > Further sched/rt.c:burn_budget() has a bool field last in a packed > struct, yielding a sizeof() that's not a multiple of 4. All the uses of > __packed there look at best suspicious anyway. Ugh - I checked the __trace_var() users, but not trace_var(). Luckily, there are far fewer of the latter. HVMOP_xentrace has no business being a hypercall in the first place. That can be fixed by also enforcing the multiple-of-4 requirement. But yes - burn_budget() needs fixing in this patch too, taking it from a theoretical to real problem. ~Andrew
