Re: [PATCH 1/6] xen/trace: Don't over-read trace objects

[Jan Beulich <jbeulich@xxxxxxxx>]

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? They may very well come without extra data
afaics.

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

Jan