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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Wed, 22 Sep 2021 13:58:31 +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=1KX05VlyxTNYGz7J4fRuoOrsB8QhhDZUG8KbfqC+gXw=; b=BSFyt4WcnKyJl8ANwYrJeoMHbP+qTJWhuTVvEHIu5t2Pj6T8fQsV6RnyYl1CotPpxym0cijhypnjLBIdJk7z/zTGxN6izQuq5TUukg9GHA+gARayck2rKbVwq7UKwyUrkEJ0yRw339/bahJtlh5GWY9JFSQWQZGp6NcyQpuVXpwzixcTO4+f1rS+Wy1kODtOn9z/5nTaSQ6qXhnnSdQ8GklgqYYFH3XSvX9iBgFfZ75worwMQb+48Wbln1dyJPlBfdU3iOCtsa45/eWCZea4uLhxkgO6jWW+U7X8NRi+Q9jpdXaMpRVEQlvKeW58x1ppq4ExQ+vk7E7m5rqWoLpoGQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dRkjtH3WKoDiarzFD7fkkvbHRthIRib0/s10ECaT9kNJwC9C9/J2zCcZH0jy3AWUf+F+JZoBU2u7rBvnWwBXkndAdECximjic5aGrqmTY2H4fmuWUJIBqG2zy8oCGLnOxMyb2vzKaQmHLq0jDR3p38tOWbVEz1oNi+uoADegbIuxfzFpSoHHjIRx5gRkEUc7gQ5XAiyXUIJTiGMKqn9oGdOZ+FGKL5EepmYUXeq4Xu1vPUZqzVGxpJ/3d/2AW7YA+0NsQn8oAhFJqAxZfzGiQ9BKtTez9hNTjMqKNry1YE+iQDjaQcjnDy6fUG1efmXV0rfXmNqT482QMavsEplXiA==
  • Authentication-results: esa1.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>, Meng Xu <mengxu@xxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 22 Sep 2021 12:58:58 +0000
  • Ironport-data: A9a23:Oykn0K6GERvRQOwLFnWdigxRtKfAchMFZxGqfqrLsTDasY5as4F+v msaWj2FMvzfYjbwc9Agb97l9B9Uu5eEyYUwGwpu+Xo1Hi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuV3zyIQUBUjclkfJKlYAL/En03FVIMpBsJ00o5wrZo2NQw2rBVPivW0 T/Mi5yHULOa82Yc3lI8s8pvfzs24ZweEBtB1rAPTagjUG32zhH5P7pGTU2FFFPqQ5E8IwKPb 72rIIdVXI/u10xF5tuNyt4Xe6CRK1LYFVDmZnF+A8BOjvXez8CbP2lS2Pc0MC9qZzu1c99Z8 9pHjYKwYggQMrDLvd0QbgN8On5AIvgTkFPHCSDXXc27ykTHdz3nwul0DVFwNoodkgp1KTgQr 7pCcmlLN03dwbLtqF64YrAEasALBc/nJo4A/FpnyinUF60OSpHfWaTao9Rf2V/cg+gTRqmFP ZZJNFKDajzrSQVQMwc8EKsup/ayr1fkaRdklQ6s8P9fD2/7k1UqjemF3MDuUseRWcxfk0Kcp 2TH12f0GBcXMJqY0zXt2m2orv/Cm2X8Qo16PL+y++NugVaT7ncOExBQXly+ydG4lUyWS99ZM 1YT+Cclse417kPDZtzwWRKovVaPvwVaRsJdFet85Q2QooLE7gDcCmUaQzppbN09qNRwVTEsz kWOnd7iGXpoqrL9YXCA8raZqxuiNC5TKnUNDQcIQwIK7NjkpIAblQ/UQ5BoF6vdszHuMWium XbQ9nF43uhNy55Qv0mmwbzZqwmjrKDXTiU63wnKWUec4z5jQKf6WZP9vDA38s18wJalokip5 SZfwZHOsrxWVPlhhwTWH75cR+jBC+KtdWSG2A8xRcFJGyGFpib7Fb289g2SM6uA3iwsQjbvf EabkgdY/pY70JCCPPIvPt7Z5yjHy8Hd+TXZuhL8NYEmjntZLlbvEMRSiam4hTuFraTUuftjU ap3iO71ZZrgNUiC8NZRb7xHuYLHOwhknT+DLXwF503/jNJym0J5uZ9aaQDTP4jVHYuvoRnP8 sY3Cid540wEC4XDjt3s2ddLdzgidCFjbbiv8pA/XrPTc2JORTB6Y9eMkOxJRmCQt/kM/gs+1 irmAREwJZuWrSCvFDhmnVg5OeuzAs4g/SxkVcHuVH7xs0UejU+UxP53X7M8fKU99fwlyvhxT vIffN6HDOgJQTPCkwnxp7GnxGC7XBj01w+IIQS/Zz0zI8xpSwDTo4e2dQrz7igeSCGwsJJm8 bGn0wraR7sFRhhjU5mKOK7+kQvpsChPgv92UmvJPsJXJBfm/r91JnGjlfQwOcwNd0nOn2PIy waMDB4EjuDRuItposLRjKWJot7xQetzF0ZXBUfB6rOyOXWI92av29YYAu2JYSrcRCX//6D7P bdZyPT1MfsmmldWstUjT+Y3nPxmv9a2/u1U1AVpGnnPfm+HMLI4LynUx9RLu41M2qRd5Vm8V HWQ94QIIr6OIs7kTgIcfVJ3cuSZ2PgIsTDO9vBpcl7i7Sp68bfbA0VfOx6A1H5UILdvad53x O4gvIgd6hCliwpsOdGD13gG+2OJJ30GcqMmqpBFX9O71lt1kglPMc7GFyv7wJCTcNEdYEAlL widiLfGm7kBlFHJdGA+FCSV0OdQ7XjUVMumELPWy4y1p+f4
  • Ironport-hdrordr: A9a23:k4X5M62sC3UtjNmzH7DT8gqjBIwkLtp133Aq2lEZdPUzSL3+qy nOpoV+6faaskdyZJhNo7C90cq7IE80l6QFh7X5VI3KNGOK1FdARLsSlLcKqAeQfhEWmNQttp uIWpIOcOEZUjNB5voSmjPXLz+L+qj9zEnSv4jj80s=
  • Ironport-sdr: OOP/gFVglvDWMjKEvtNjA0LTISCombTNPUTBNwh9sXsGF0iWOyU/lyeqhgz7sUpJxHLHvK9b+/ fep9+rYRXesj6k14319Lo1Dw6XRs2OR1snIFrVwH/IsHlBQ8T2qCcnxzESG/2WclLB01A1RKbH p1NMEmseS0h+2Yp7XqA0gS4VEPU7nSxLL3xKtYhG1ZdqH/A6G7/w+YGJbZNURF2l4FxBnlXnVE oFdAIPtuY40Pb7ijJopqWgO4Q+2ZiOEtGqOo0QwSskteIsUpRbBACJQ8sZQuSVzQzCN51id9cf qGXG5nLWBb4Ly5HS0JNyykax
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 22/09/2021 08:01, Jan Beulich wrote:
> On 21.09.2021 19:51, Andrew Cooper wrote:
>> On 21/09/2021 07:53, Jan Beulich wrote:
>>> On 20.09.2021 19:25, Andrew Cooper wrote:
>>>> --- 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;
>>> Despite this being a function that supposedly no-one is to really
>>> use, you're breaking the interface here when really there would be a
>>> way to be backwards compatible: Instead of failing, pad the data to
>>> a 32-bit boundary. As the interface struct is large enough, this
>>> would look to be as simple as a memset() plus aligning extra_bytes
>>> upwards. Otherwise the deliberate breaking of potential existing
>>> callers wants making explicit in the respective paragraph of the
>>> description.
>> It is discussed, along with a justification for why an ABI change is fine.
> What you say is "This has no business being a hypercall in the first
> place", yet to me that's not a justification to break an interface.

No, but "cannot be used outside of custom debugging" means there are no
users in practice, and therefore it really doesn't matter.

> It is part of the ABI, so disallowing what was previously allowed
> may break people's (questionable, yes) code.
>
>> But I could do
>>
>> tr.extra_bytes = ROUNDUP(tr.extra_bytes, sizeof(uint32_t));
>>
>> if you'd really prefer.
> I would, indeed, and as said ideally alongside clearing the excess
> bytes in the buffer.

Why?  The entire structure is copied out of guest memory, with a fixed size.

It's not Xen's fault/problem if the VM didn't initialise it correctly,
and an explicit ROUNDUP() here maintains the current behaviour.

>>>> --- 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;
>>> The original field was plain int, and aiui for a valid reason. I
>>> don't see why you couldn't use int32_t here.
>> delta can't be negative, because there is a check earlier in the function.
> Oh, yes, didn't spot that.
>
>> What is a problem is the 63=>32 bit truncation, and uint32_t here is
>> half as bad as int32_t.
> Agreed. Whether the truncation is an issue in practice is questionable,
> as I wouldn't expect budget to be consumed in multiple-second individual
> steps. But I didn't check whether this scheduler might allow a vCPU to
> run for this long all in one go.

I expect it's marginal too.  Honestly, its not a bug I care to fix right
about now.  I could leave a /* TODO: truncation? */ in place so whomever
encounters weird behaviour from this trace record has a bit more help of
where to look?

~Andrew




 


Rackspace

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