| Message ID | 20210917084559.22673-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/trace: Fix leakage of uninitialised stack into the tracebuffer | expand |
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
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
On 17.09.2021 15:26, Andrew Cooper wrote: > 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. I was about to say that the two parts of your reply contradict one another, when I finally realized that it looks like the first sentence in the comment can be read two ways: "Trace records require extra data" then going on to describe properties, or "Trace records require extra data to be an exact multiple of uint32_t." Obviously this is to me as a non-native speaker. But maybe you could still reword this to be unambiguous? (I'm not going to exclude that the lack of a comma, which I did silently add while reading, makes a difference here: Does "Trace records require extra data, which is an exact multiple of uint32_t" end up altering the meaning?) Jan
On 20/09/2021 09:00, Jan Beulich wrote: > On 17.09.2021 15:26, Andrew Cooper wrote: >> 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. > I was about to say that the two parts of your reply contradict one > another, when I finally realized that it looks like the first sentence > in the comment can be read two ways: "Trace records require extra data" > then going on to describe properties, or "Trace records require extra > data to be an exact multiple of uint32_t." Obviously this is to me as a > non-native speaker. But maybe you could still reword this to be > unambiguous? (I'm not going to exclude that the lack of a comma, which > I did silently add while reading, makes a difference here: Does "Trace > records require extra data, which is an exact multiple of uint32_t" end > up altering the meaning?) Yes. The requirement is for "extra data which is an exact multiple of uint32_t", not "extra data". The comma massively changes the meaning. I'll see about tweaking the wording. ~Andrew
diff --git a/xen/common/trace.c b/xen/common/trace.c index a2a389a1c7c3..25af6e1bd25e 100644 --- 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. + */ + 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); if ( (tb_event_mask & event) == 0 ) return;
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. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <iwj@xenproject.org> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> CC: Dario Faggioli <dfaggioli@suse.com> I've eyeballed the code and can't spot any problematic callers, but I came very close to accidentally introducing some when trying to fix the stack rubble leaks in subsequent patches. --- xen/common/trace.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)