[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Fri, 17 Sep 2021 14:26:35 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=HZAdBgS4/6chgWS5jvU7DTQYRfcpq5y/k6AEYNbo5OQ=; b=T2kiObiJCtbwxy1pE0zsXvHV7nyLyls1vyzvRsx4SUA3UfKm3w2hLa/EqCnb0/+64mEZvRsUix6jFac6JtZ5aZhe0jW7e2lYB6nv3aD0hH15pT4PeiQZpqsbJ61nRRMK4TxFttny1SZju423BRG0B1cFWfgekpQfpKNgVuKvTZgQOyck+gcyvYfQo4/gvZSr652wq8wWnKVhWU7hT+kGSFUF3ZDDJpMl86sZ60DtMyGkPfNSS5Wficug/RbLHovmz5v2x+rjLMqfw8XEDp5zIM+VBzs6j3WyG1Y8JjnT4uL7TN7EYpZ2gkyznHsAqxg7PaJ8Z4K2i7DROtDgDmSgvQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Wej+XIEDC+j8pfX95/w6gfHQsdKGpOO1ol1WeA5x6MIWve1oJzDM1ef3d7RAJVghK8iv5b1tkIpwuekV/gJph+QOmwmMqhhuRmn87qfT1GfyHefwYA2ybWRaQJTUxnSluCjhu5hOZA7XIFFrJG72qmeBaJHztU/Canudv/z4PgNFOEFZVz+60M90Pg8PM8mYADF2YzHODobChWiV8cXUxWf6v1tsaWiOH4rF8stiBvHM65myWX/ASpPx/cPH5EHOeNLlVtUtQ48KvpjbZ9Nk7cM7j5vDmkH5ATbXuUDpH1Ek8NffOEZT9KQN6msPZTtA1/VZXpFQazPu2ChBGgwhaw==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Dario Faggioli <dfaggioli@xxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 17 Sep 2021 13:26:59 +0000
  • Ironport-data: A9a23:J4c7gKjjr3PMqOZsPCzDJqjFX1614RcKZh0ujC45NGQN5FlHY01je htvUWCGP/2MYGrzfdp2at629h8FvMeGnd5qHFE//ng2Fi4b9cadCdqndUqhZCn6wu8v7a5EA 2fyTvGacajYm1eF/k/F3oAMKRCQ7InQLlbGILes1htZGEk0FU/NtTo5w7Rg2t8x3oDja++wk YiaT/P3aQfNNwFcagr424rbwP+4lK2v0N+wlgVWicFj5DcypVFMZH4sDfjZw0/DaptVBoaHq 9Prl9lVyI97EyAFUbtJmp6jGqEDryW70QKm0hK6UID66vROS7BbPg/W+5PwZG8O4whlkeydx /0Op6GcFjoMEpaQt84xAid+IgJBfo1ZreqvzXiX6aR/zmXDenrohf5vEFs3LcsT/eMf7WNmr KJCbmpXN1ba2rzwkOnTpupE36zPKOHCOo8Ft24m5jbeFfs8GrjIQrnQ5M8e1zA17ixLNauEN pVCMGY3BPjGSwFhCk4wE6Ngp+6h2X3aWR129U2sopNitgA/yyQuieOwYbI5YOeiXt5Jl0yVo mbH+WXRARwAMtGbjz2f/RqEh/DNtTP2XpoIE7+1/eIsh0ecrkQRAhALUVqwodGil1WzHdlYL iQ85S4GvaU0skuxQbHVTxC+5XKJoBMYc95RCPEhrhGAzLLO5ASUDXRCSSROAPQqusY5Sj0t0 l6hhM7yCHpkt7j9YXCA8raZqxuiNC5TKnUNDRLoViNcvYOl+ttqyEuSEJAzS8ZZk+EZBxmz0 Q2m8xUbq45PouQnhpudzHP4wBKz882hohEO2unHYo60xlonP9f1PN35sQKzAeVod9nCHwLY1 JQQs43HtrlfU8vV/MCYaLhVRNmUC+C53CowaLKFN6Io8Siks1WndJpZiN2VDBY0aptYEdMFj Um6hO+w2HOxFCDxBUOUS9joYyjP8UQHPY6+Ps04lvIUPvBMmPavpUmCn3J8OlwBd2B3yskC1 WqzK57wXR7294w+lGfeqxghPU8DmXllmDK7qWHT5BW7y7uODEN5up9cawDmUwzN14vd+F+92 48Gb6OikkwDOMWjMni/2dNCdjgicClkba0aXuQKL4Zv1CI9Qzp/YxIQqJt8E7FYc1N9zbuRp SDhBRYDkTISRxTvcG23V5yqU5u2Nb5XpnMnJy08e1Gu3nkoe4G066kDMZAweNEaGCZLl5aYl tEJJJeNBOphUDPC92hPZJXxttU6Jh+qmRiPL2yuZz1mJ8xsQAnA+9nFeArz9XZRUnrr5JVm+ 7DwhBnGRZcjRhh5CJqEYvyY0F7s72MWn/h/XhWUL4ALKlns6oVjNwf4kuQzf5MXMRzGyzbDj 1SWDB4UqPPjuYgw9NWV16mIo53wS7l1H1ZAHnmd5rGzbHGI8m2myI5GceCJYTGCCz+kpPT8P b1YlqiuPucGkVBGt5tHP4xqla9utcHyo7J6zxh/GCmZZVqcFb49cGKN2tNCt/MRy+YB6xe2Q E+G5vJTJa6NZJH+CFcUKQcoMraD2PUTlmWA5Pg5Ohyntip+/b7BWkROJRiczidaKeItYo8ix O4gvu8Q6hC+1UV2YorX0HgM+jTeNGEEXoUmqooeUd3ihQccw11fZYDRV33t65aVZtQQakQnL 1d4XkYZa2iwEqYaT0cOKA==
  • Ironport-hdrordr: A9a23:5TPCFqmoZJ/fgjPGUeWZJ5H1QNrpDfIo3DAbv31ZSRFFG/Fw8P re+8jztCWE7Ar5PUtKpTnuAsW9qB/nmqKdgrNwAV7BZmfbUQKTRekJgLcKqAeAJwTOssJbyK d8Y+xfJbTLfD1HZB/BkWqF+gAbsbu6zJw=
  • Ironport-sdr: YG5Mi/Q92JDBauIHSxUBmJ/9rE47i0uAK1qTmwdFC8CDb2gpdcunmfLg64rTQrFHoo2j7e4Unw g5vWh6Hz6KrgSFehheVFFvP3Fup47JEu0F38Nno1bfuPOLoZ2M2N30A+QMGlyDPXNHU9VpO5Ea IF7VMxTJ2K7l6yax8pwR78AoTZYUNBVzOO2XWiRF7fYjkHCnt6P/h5cVXcXq57r/ky/2XZEvu8 xnXA878jf+Nm0B+6xOtkv0ObhEg+d8aAsuTr4MgpzVASaMbtAJ0SFIjFBG1j+L2GHOegMoWkz4 QkC1Rs9znEegOA7zj9Gn+Mbs
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.