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

Re: [PATCH 2/4] x86/P2M: relax guarding of MMIO entries


  • To: Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Tue, 31 Aug 2021 14:16:43 +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:X-MS-Exchange-SenderADCheck; bh=qbmiHIXfPxn0P76W+oaSNoaoF/UoozDovukYELV472o=; b=UhPUGSaZ3nPiB4cJBAMc99++pyMxARRLWNR49+hpOYN9kkLZbS+31mRPQ9LfgyYw/ZAkBt5mJEIE7aR3tAFz0gKVje24UvfsdEKD9/JF6cbMvsZbUoRC7maID4BUUxhFWrXuFfPj8t5wFKUj7yPnmk50iU8auOsKC5aQsknsOoDcCBbHG+6GqrmrYogtNgat5UneQXbzDNvPkowQr5lT6hlQuS6cH6UHhAcPpmVjLBV8dlIpF4esVycG+Rffyxb6Ogp6t4byE7kWS/eRExByUw2wPI8cpYw+VnlXTTmZL63W07b/erxdCfrLI3T0Rphv8pXKd8769a/3dG4nqUPRaA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=b+grZUlv+KKA8BC1xFJAitlmVbw4aDs9t38js2/OqsFFEhsDlvFtPrGfu9YASqmwIEFSu76PEeDupUDMOQR0OAKULRIXiNJ6W/KG3ddWLeeyDxSCbZFHRuMXJTMOJ/OgATNE2lc9njOmCWm8TOhm58Vcatnt8kGBE+4wS1B9YuZ42G1olEB6gLXopAbPRt1ub4s1fAS5E3QuJ4vKv967xM6vS1FnUnj3Hapjdmv6ApqjIbRjdMPKKqe91qgYw6bbq9ydQFlTCIrxc45B0bWWy/1NKJtWIZBBr8PJSbevE8APbH0klTm7FUuxrSNyYL4AkROgymrRDboyGRnGtFywNw==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Tue, 31 Aug 2021 13:16:59 +0000
  • Ironport-hdrordr: A9a23:upaQKKg3Hv62lNCt6c0ieo5MvXBQX1R13DAbv31ZSRFFG/FwyP rAoB1L73PJYWgqNU3I+ergBEGBKUmskKKdkrNhQotKOzOWxFdATbsSkLcKpgePJ8SQzJ8k6U 4NSdkYNDS0NykBsS+Y2njJLz9D+qj/zEnAv463pB0MPGIaGZ2IrT0JbjpzencGNTWubqBJcq Z0iPA3xQZINU5nFfhSURI+Lpb+TpDw5d3bSC9DIyRixBiFjDuu5rK/Ox+E3i0GWzcK5bs562 DKnyHw+63m6piAu17h/l6Wy64TtMrqy9NFCsDJos8JKg/0ggLtQIh6QbWNsB08venqwlc3l9 vnpQsmIq1Imj3sV1DwhSGo9xjr0T4o5XOn4ViEgUH7qci8fz4+A9opv/MSTjLpr24b+P1s2q NC2GyU87BNCwnboSj779/UEzl3i0uduxMZ4Kwupk0adbFbRK5arIQZ8k8QOowHBjjG5IcuF/ QrJN3A5cxRbUiRYxnizypSKeSXLzAO9yq9Mw8/UpT/6UkRoJk59TpZ+CUnpAZEyHpnIKM0vt gtW89T5cJzpsx/V9M3OA5Oe7ruNoRhKSi8Rl56Gm6XYJ3vDUi946If0I9Fkd1CR6Z4u6fauK 6xHW+w5lRCN34HN6W1rdR2G1b2MT6AYQg=
  • Ironport-sdr: Ef7OwBayHxTriIFYmw9qDdyw6zabZz2aiJ7CO2YcP/KSgSDpRATBLu7glF10HzZxqC4ANwi0bZ ieBN4zVZWm9i2Vdvjxl6l5h/PepQkrMJ/m+YBEXzDUinEV9nOfx7RQMUpKAGsQd/i5+Jdsjks0 v/3+vB1SDCMalbIixCSgI0Z2BNn8QV9UFcFHpVS2ibVeHVwmpp4SVfq7t1M61aKmDcM/iMtm1F 74YXxXsnireiQDo3cW6Ids512VpIxNPHbLf0fp4eAOPINqwQcX5z4QH7Tm4OgtDutXwu0BQPcj PoIJ3X1mibDY10vfwxRiuaJk
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 30/08/2021 14:02, Jan Beulich wrote:
> One of the changes comprising the fixes for XSA-378 disallows replacing
> MMIO mappings by unintended (for this purpose) code paths.

Drop the brackets.

> At least in
> the case of PVH Dom0 hitting an RMRR covered by an E820 ACPI region,
> this is too strict. Generally short-circuit requests establishing the
> same kind of mapping that's already in place.
>
> Further permit "access" to differ in the "executable" attribute. While
> ideally only ROM regions would get mapped with X set, getting there is
> quite a bit of work. Therefore, as a temporary measure, permit X to
> vary. For Dom0 the more permissive of the types will be used, while for
> DomU it'll be the more restrictive one.

Split behaviour between dom0 and domU based on types alone cannot
possibly be correct.

DomU's need to execute ROMs too, and this looks like will malfunction if
a ROM ends up in the region that HVMLoader relocated RAM from.

As this is a temporary bodge emergency bugfix, don't try to be clever -
just take the latest access.

> While there, also add a log message to the other domain_crash()
> invocation that did prevent PVH Dom0 from coming up after the XSA-378
> changes.
>
> Fixes: 753cb68e6530 ("x86/p2m: guard (in particular) identity mapping 
> entries")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -958,9 +958,13 @@ guest_physmap_add_entry(struct domain *d
>          if ( p2m_is_special(ot) )
>          {
>              /* Don't permit unmapping grant/foreign/direct-MMIO this way. */
> -            domain_crash(d);
>              p2m_unlock(p2m);
> -            
> +            printk(XENLOG_G_ERR
> +                   "%pd: GFN %lx (%lx:%u:%u) -> (%lx:%u:%u) not permitted\n",

type and access need to be rendered in hex, or you need to use 0x
prefixes to distinguish the two bases.

Also, use commas rather than colons.  Visually, this is ambiguous with
PCI BDFs, and commas match tuple notation in most programming languages
which is the construct you're trying to represent.

Same below.

> +                   d, gfn_x(gfn) + i,
> +                   mfn_x(omfn), ot, a,
> +                   mfn_x(mfn) + i, t, p2m->default_access);
> +            domain_crash(d);
>              return -EPERM;
>          }
>          else if ( p2m_is_ram(ot) && !p2m_is_paged(ot) )
> @@ -1302,9 +1306,50 @@ static int set_typed_p2m_entry(struct do
>      }
>      if ( p2m_is_special(ot) )
>      {
> -        gfn_unlock(p2m, gfn, order);
> -        domain_crash(d);
> -        return -EPERM;
> +        bool done = false, bad = true;
> +
> +        /* Special-case (almost) identical mappings. */
> +        if ( mfn_eq(mfn, omfn) && gfn_p2mt == ot )
> +        {
> +            /*
> +             * For MMIO allow X to differ in the requests (to cover for
> +             * set_identity_p2m_entry() and set_mmio_p2m_entry() differing in
> +             * the way they specify "access"). For the hardware domain put 
> (or
> +             * leave) in place the more permissive of the two possibilities,
> +             * while for DomU-s go with the more restrictive variant.

This comment needs to identify clearly that it is a temporary bodge
intended to be removed.

~Andrew

> +             */
> +            if ( gfn_p2mt == p2m_mmio_direct &&
> +                 access <= p2m_access_rwx &&
> +                 (access ^ a) == p2m_access_x )
> +            {
> +                if ( is_hardware_domain(d) )
> +                    access |= p2m_access_x;
> +                else
> +                    access &= ~p2m_access_x;
> +                bad = access == p2m_access_n;
> +            }
> +
> +            if ( access == a )
> +                done = true;
> +        }
> +
> +        if ( done )
> +        {
> +            gfn_unlock(p2m, gfn, order);
> +            return 0;
> +        }
> +
> +        if ( bad )
> +        {
> +            gfn_unlock(p2m, gfn, order);
> +            printk(XENLOG_G_ERR
> +                   "%pd: GFN %lx (%lx:%u:%u:%u) -> (%lx:%u:%u:%u) not 
> permitted\n",
> +                   d, gfn_l,
> +                   mfn_x(omfn), cur_order, ot, a,
> +                   mfn_x(mfn), order, gfn_p2mt, access);
> +            domain_crash(d);
> +            return -EPERM;
> +        }
>      }
>      else if ( p2m_is_ram(ot) )
>      {
>





 


Rackspace

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