[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: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 22 Sep 2021 09:01:00 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=6PhIu2Syyiorg/TJVOVOlcj2U13qurfGV+jCJIahXck=; b=NRMWjLP3/SEb0oqsTfgjRmE+f6uoC5eJ6heNB1UFzfGgJ6BkOIJF5XEaVYkkLP6ZoNSugkR/keRoXYnQC3XaKS7ANHR1uVxSYBJiP7ba1x8I0wcvER02TQrn07YhGi9+f0XFcjn2jedP6TCnbF9zvS2kTVbPlgdoXyQ+p4JgkEqaugR7MR++hUvCmEsfCZ4f/v3/trpZHFA8qFVh6L3nMooFf/SbMbzzcnphhDZA+PDzY85z/IaEYqrlFshE7zkNeqO2rex0naLdxSzwxqXt1UrILJsi6kTGX85cj02isu/BaxrJKu8cSWOCAXA+uPiuHOG7Bm2jqZBjryOlU+dijQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=G0jVUQ9xwgruqN+aEn0/jZ216/oxB9UMbThnkSFsrs+TNV0RAEDwX7qeUe51/4I7ZSgsR+mAuZ0s7IYKrKJcqJ+zXBUsDRfUQjsM3aqZkbm1/zxhuSzKI1MqOHo6gPWJXuFI5eqfvBg34dskVRJjyl/caktAMDflahys9OvrOzZPhNLfZVcwCecTfytprX+60llFuQZdtRKo1PpKxbKDX/IySUKPPU84uN3IZAkMFmVX+uiCmn95SeBoKi7Uw6/MTwC57gNKXLAzIDmPRZViLSqzstDEq9fYrTCY4jX0LgZwvizT3YJU4+8srxfgo3I/Yxjn7sVO1zne4a+k0eQlqA==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.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 07:01:16 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.
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.

>> As an aside I think at the very least the case block wants enclosing
>> in "#ifdef CONFIG_TRACEBUFFER", such that builds without the support
>> would indicate so to callers (albeit that indication would then be
>> accompanied by a bogus log message in debug builds).
> 
> That message really needs deleting.
> 
> As a better alternative,
> 
> if ( !IS_ENABLED(CONFIG_TRACEBUFFER) )
>     return -EOPNOTSUPP;
> 
> The call to __trace_var() is safe in !CONFIG_TRACEBUFFER builds.

Fine with me (I'm inclined to say of course).

>> Seeing the adjacent HVMOP_get_time I also wonder who the intended
>> users of that one are.
> 
> There is a very large amount of junk in hvm_op(), and to a first
> approximation, I would include HVMOP_get_time in that category.
> 
> But c/s b91391491c58ac40a935e10cf0703b87d8733c38 explains why it is
> necessary.  This just goes to demonstrate how broken our time handling
> is.  I'll add this to the pile of things needing fixing in ABI-v2.

Urgh.

>>> --- 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.

Jan




 


Rackspace

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