[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>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 1 Sep 2021 15:48:24 +0200
  • 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=0uko84/08g7wVwgcKSsgJA9ORW/fkmuAIxaRx9UBpIw=; b=Uj8PqFGqRk7J8OifrZ9KfBd0j+3oqnQS7sgKSgUA4CMt3t2NQrlaQm5Eybjnl/ht1J42QWZkvMrOGlYLX7cH9F7Y61gwl4jVdSlUNHalGrZZXJ2yrPWul5Yt9RxVz+tZtoQKNIWo7lzroLgKU1iIU43mUdRcfGdpneF7CLVCnhHLULDzRgWWjxdjyBuJJeivpKxagzI7sxXMTEtdK4WayYg+grgbTgl+pOofu4mKHkvrMrnUpYtYPctpv+E8rRWk0t4GASMQ0NubXf8SskkNy8swP0VHbg7YZn9VSbfnBU895VUBlwKpUNF6WQ2vxiK+WwJe8llMNyTgXSwMzb69yw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Hni2p7i6o8Fc8nMMgBLTmo2PuGG3ZzI2f1YKH8ioqFl5Y0hawo8p/+r3zXsx25d7brFFws0ha+mm6IjE7Z3vaADbFh2tg+sl6ZEIb4Z5ggWsJSvdUZjj6tYR/FCczpC233kYkuXL8coYz3rKgqc+fuZPu4VYF8ycxodSUaDeaZRo3P0oRZmx/jgWilBJssKI4uwmKVWzpmDtbUdK79ra1BJIdzi6mtr34rlQNGUG+CmlDyUCmELKndXSFxqQdmw26aofilFlHCweTiHaB+wQZNBgc9Aa6u1e1JXtfpB9ljK5s8w1wS4FtWwAdeNA5j9VFhAZffFrjCaYi9/3Iw6R7w==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 01 Sep 2021 13:48:57 +0000
  • Ironport-hdrordr: A9a23:+WB0G6P5PJ+DH8BcT1L155DYdb4zR+YMi2TDiHoedfUFSKOlfp 6V8MjztSWVtN4QMEtQ/+xoHJPwPE80kqQFnbX5XI3SJjUO3VHIEGgM1/qG/9SNIVybygcZ79 YeT0EcMqyBMbEZt7eD3ODQKb9Jq7PrgcPY55as854ud3AQV0gJ1XYJNu/xKDwOeOApP+tfKH LKjfA32QZINE5nIviTNz0gZazuttfLnJXpbVovAAMm0hCHiXeN5KThGxaV8x8CW3cXqI1Sv1 Ttokjc3OGOovu7whjT2yv66IlXosLozp9mCNaXgsYYBz3wgkKDZZhnWZeFoDcpydvfp2oCoZ 3pmVMNLs5z43TeciWcpgbs4RDp1HIU53rr2Taj8DDeiP28YAh/J9tKhIpffBecwVEnpstA3K VC2H/cn4ZLDDvb9R6NpOTgZlVPrA6ZsHAimekcgzh0So0FcoJcqoQZ4Qd8DIoAJiTn84oqed MeTP003MwmNG9yUkqp+lWGmLeXLzMO91a9Mwk/U/WuonprdCsT9Tpf+CQd9k1wvK7VBaM0vt gtn8xT5cZzp/QtHNdA7dE6MIKK41z2MGDx2V2pUCDa/YE8SjjwQs3MkfgIDN/DQu1/8HJ1ou WYbG9l
  • Ironport-sdr: jPi5RsrDq8vGxVA0R5jGxt5cenwvNBfmBZGDQMKiE1AFYKURsTUmSTbwQra4o5dyL59hBeY8ZK 4idzOWHFaBF7g8iREyL1Yjhi9Pq/4Et2hc3Ae04KWUWZc+jd14hUvsxjO1y9Ai0mtOvwf7MtyG +d/olGD96J2WM3+a3UCASXQz4xcTZ6Wb9XCs5f2emQn5Vozut7L8Cu1k643/A8easgzRZnWRNb ElncjzE7/9IpFudkEtNW9GkpgvOffpKe6SHHq+gtpTOkQPmiLt7tJUwmRnK6XSyJ+ht6Q3pJB0 mMQOBniVUtDseSIDuJ68S+49
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, Sep 01, 2021 at 11:53:03AM +0200, Jan Beulich wrote:
> On 01.09.2021 10:50, Roger Pau Monné wrote:
> > On Tue, Aug 31, 2021 at 05:38:49PM +0200, Jan Beulich wrote:
> >> On 31.08.2021 17:25, Andrew Cooper wrote:
> >>> On 31/08/2021 14:26, Jan Beulich wrote:
> >>>> On 31.08.2021 15:16, Andrew Cooper wrote:
> >>>>> On 30/08/2021 14:02, Jan Beulich wrote:
> >>>>>> 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.
> >>>> True, but what do you do.
> >>>>
> >>>>> 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.
> >>>> And how do we know that that's what is going to work?
> >>>
> >>> Because it's the pre-existing behaviour.
> >>
> >> Valid point. But for the DomU case there simply has not been any
> >> pre-existing behavior. Hence my desire to be restrictive initially
> >> there.
> >>
> >>>>  We should
> >>>> strictly accumulate for Dom0. And what we do for DomU is moot for
> >>>> the moment, until PCI passthrough becomes a thing for PVH. Hence
> >>>> I've opted to be restrictive there - I'd rather see things break
> >>>> (and getting adjusted) when this future work actually gets carried
> >>>> out, than leave things permissive for no-one to notice that it's
> >>>> too permissive, leading to an XSA.
> >>>
> >>> Restricting execute permissions is something unique to virt.  It doesn't
> >>> exist in a non-virtualised system, as I and D side reads are
> >>> indistinguishable outside of the core.
> >>>
> >>> Furthermore, it is inexpressible on some systems/configurations.
> >>>
> >>> Introspection is the only technology which should be restricting execute
> >>> permissions in the p2m, and only when it takes responsibility for
> >>> dealing with the fallout.
> >>
> >> IOW are you saying that the calls to set_identity_p2m_entry()
> >> (pre-dating XSA-378) were wrong to use p2m_access_rw? Because that's
> >> what's getting the way here.
> > 
> > I did wonder this before, because I saw some messages on a couple of
> > systems about mappings override, and I'm not sure why we need to use
> > p2m_access_rw. My first thought was to suggest to switch to use the
> > default access type for the domain, like set_mmio_p2m_entry does.
> > 
> > I have to admit I'm not sure I see the point of preventing execution,
> > but it's possible I'm missing something.
> Well, what good can come from allowing execution from, say, the
> IO-APIC or LAPIC pages? Or other MMIO-mapped register space?

map_mmio_regions does already map BARs with execute permissions, so
it's just some MMIO regions that get mapped without execution
permissions, which makes all this confusing.

> Insn
> fetches might even trip bad hardware behavior in such a case by
> being the wrong granularity.

Normal reads could also trigger such bad hardware behavior, so I'm not
sure preventing execution provides Xen any more safety.

> It's imo really only ROM space which
> ought to have execution permitted.
> The issue isn't just with execution, though, and as a result I may
> need to change the logic here to also include at least W. As of
> one of the XSA-378 changes we may now pass just p2m_access_r to
> iommu_identity_mapping(), if the ACPI tables on an AMD system were
> saying so. (We may also pass p2m_access_w, but I sincerely hope no
> firmware would specify write but no read access.)
> Similarly in "IOMMU/x86: restrict IO-APIC mappings for PV Dom0" I
> now pass p2m_access_r to set_identity_p2m_entry().

Not really I think, as PVH dom0 is the only user of the
set_identity_p2m_entry call in arch_iommu_hwdom_init, and we should
never identity map the IO-APIC range in that case because a set of
emulated IO-APIC replacements are provided and those require ranges to
be unmapped so that accesses can be trapped.

> I suppose an underlying issue is the mixed purpose of using
> p2m_access_*, which possibly has been against the intentions in the
> first place. We cannot, for example, express r/o access to an MMIO
> page without using p2m_access_r (or p2m_access_rx), as there's no
> suitable p2m type to express this via type alone. We may need to
> split p2m_mmio_direct into multiple types (up to 7), I guess, if
> we wanted to remove this (ab)use of p2m_access_*.

My main complaint is mostly with the fact that some MMIO ranges are
mapped without execute permissions when mapped by
set_identity_p2m_entry vs map_mmio_regions that will map them with the
default permissions and that has execution set.

If the mappings from arch_iommu_hwdom_init for example would use the
default permissions that could solve quite a lot of the issues there




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