All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>,
	Ian Jackson <iwj@xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Julien Grall <julien@xen.org>,
	Dario Faggioli <dfaggioli@suse.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 1/6] xen/trace: Don't over-read trace objects
Date: Fri, 17 Sep 2021 14:26:35 +0100	[thread overview]
Message-ID: <0db4dc68-ccf0-0de6-61ce-7f155431a977@citrix.com> (raw)
In-Reply-To: <ed07d2de-9354-6c6d-b9f6-4b325ca328ce@suse.com>

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



  reply	other threads:[~2021-09-17 13:27 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-17  8:45 [PATCH 0/6] xen/trace: Fix leakage of uninitialised stack into the tracebuffer Andrew Cooper
2021-09-17  8:45 ` [PATCH 1/6] xen/trace: Don't over-read trace objects Andrew Cooper
2021-09-17 12:58   ` Jan Beulich
2021-09-17 13:26     ` Andrew Cooper [this message]
2021-09-20  8:00       ` Jan Beulich
2021-09-20 10:24         ` Andrew Cooper
2021-09-17  8:45 ` [PATCH 2/6] xen/memory: Remove tail padding from TRC_MEM_* records Andrew Cooper
2021-09-17 13:04   ` Jan Beulich
2021-09-17  8:45 ` [PATCH 3/6] xen/credit2: Remove tail padding from TRC_CSCHED2_* records Andrew Cooper
2021-09-17 13:10   ` Jan Beulich
2021-09-17 13:28     ` Andrew Cooper
2021-09-17  8:45 ` [PATCH 4/6] x86/trace: Reduce stack usage from HVMTRACE_ND() Andrew Cooper
2021-09-20  9:05   ` Jan Beulich
2021-09-20 11:02     ` Andrew Cooper
2021-09-20 13:00       ` Jan Beulich
2021-09-17  8:45 ` [PATCH 5/6] xen/credit2: Clean up trace handling Andrew Cooper
2021-09-20  9:11   ` Jan Beulich
2021-09-17  8:45 ` [PATCH 6/6] xen/trace: Minor code cleanup Andrew Cooper
2021-09-20  9:15   ` Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0db4dc68-ccf0-0de6-61ce-7f155431a977@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=George.Dunlap@eu.citrix.com \
    --cc=dfaggioli@suse.com \
    --cc=iwj@xenproject.org \
    --cc=jbeulich@suse.com \
    --cc=julien@xen.org \
    --cc=sstabellini@kernel.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.