[PATCH v2 01/12] xen/trace: Don't over-read trace objects

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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
__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.cooper3@xxxxxxxxxx>
---
CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
CC: Ian Jackson <iwj@xxxxxxxxxxxxxx>
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Wei Liu <wl@xxxxxxx>
CC: Julien Grall <julien@xxxxxxx>
CC: Dario Faggioli <dfaggioli@xxxxxxxx>
CC: Meng Xu <mengxu@xxxxxxxxxxxxx>

v2:
 * Adjust HVMOP_xentrace to avoid hitting the printk()
 * Fix TRC_RTDS_BUDGET_BURN
 * Adjust wording in __trace_var()
---
 xen/arch/x86/hvm/hvm.c |  5 +++--
 xen/common/sched/rt.c  | 24 +++++++++++++-----------
 xen/common/trace.c     | 21 ++++++++++-----------
 3 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7b48a1b925bb..09cf6330ad26 100644
--- 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;
 
         /* Cycles will be taken at the vmexit and vmenter */
diff --git a/xen/common/sched/rt.c b/xen/common/sched/rt.c
index c24cd2ac3200..c58edca0de84 100644
--- 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;
+            uint32_t priority_level;
+            uint32_t has_extratime;
+        } d = {
+            .unit            = svc->unit->unit_id,
+            .dom             = svc->unit->domain->domain_id,
+            .cur_budget      = svc->cur_budget,
+            .delta           = delta,
+            .priority_level  = svc->priority_level,
+            .has_extratime   = !!(svc->flags & RTDS_extratime),
+        };
+
         trace_var(TRC_RTDS_BUDGET_BURN, 1,
                   sizeof(d),
                   (unsigned char *) &d);
diff --git a/xen/common/trace.c b/xen/common/trace.c
index a2a389a1c7c3..4297ff505fb9 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);
+    /*
+     * extra data needs to be an exact multiple of uint32_t to prevent the
+     * later logic over-reading the object.  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;
-- 
2.11.0