[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: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 6 Sep 2021 20:53:38 +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=JUF5ZQ9qHhiOdd1nZnSVwMywJawM174/ekfRo7ZYZ6M=; b=F+ngGyUtzO8vsg4gG2Fd07aR9CD/S0N+tGkS0MBeuHEIVagmSm75qcFAkprVXJkiryOK0eAByo94Otc1gaqXAtvr9WIT7fM4VNFtXOhrouxYYhk0BdF6q3iVpSWXoLOAyhdfruQfT9m84eS94S+p1ct1A+r72ziJNzrSenIJ5Na9FmEOcu5sCdj8gI4BSWC+nJRqSxwVXZJFSFdyzlSsUDtgcDnhCT9N1OQxQq5i3kxmT67ts+pxUkCh7P2aeZah2z7KYt7vNleQ2Z3FYgG2fEjVwDjPoNgeg4gnopD19kpm6OKqdGbIqMhojFLKdceonSa0URrNfRcUcAMv+XSsbg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PzjeqbfEaMkqdKFIf6Z7afvOHn8asoEZvQbT6Rck0lmMWwji3P1hHQhstQDSyuJ6gk+mWx9GyxsPKWGFh1cy8t9rqjSRDCN4KX+E77Yvy9nBb43mZnDEZ6MHSEOwhG2OkdZZth6NO+B0Ml0NVVZjs2IaN7/35yZHYrVSLyeLM3GvKoQJdqm+rfF4SvGW4a94UrUt6SsgES/tuxBAVr6L8JR0fpUwDOyZJ+6Pqe05wu+Xd65z2Iqsxpf7kdn7W1AhsU6tUdR+2Gknym/gLnig6B4M4dkj3d7Cvxl74WOG9z+KS84DY+qTHbtckzxVcXyQlWCI2ibmWiKxivbcfPE06w==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 06 Sep 2021 19:54:08 +0000
  • Ironport-hdrordr: A9a23:kKuF+61PuK2X2RSyQlVg2gqjBTtyeYIsimQD101hICG9Lfb3qy n+ppsmPEHP5Ar5OEtBpTiBUJPwJU80hqQFn7X5Wo3SIzUO2VHYUL2KiLGC/9SOIVyEygcw79 YHT0E6MqyMMbEYt7eI3ODbKadZ/DDvysnB7o2yvhQdL3AZV0gj1XYfNu/yKDwHeOAsP+tBKH Pz3Lsjm9PtQwVsUiztbUN1LtQr6ue7267OUFojPVoK+QOOhTSn5PrTFAWZ5A4XV3dqza05+W bIvgTl7uH72svLiyP05iv21dB7idHhwtxMCIiljdUUECzljkKNaJ56U7OPkTgpqKWE6Uoskv PLvxA8Vv4DpU/5TyWQm1/AygPg2DEh5zvLzkKZu2LqpYjDSDczG6N69MhkWyqcz3BlkMB30a pN0W7cnYFQFwn8kCP04MWNfw12l2KvyEBS09I7vjh6a88zebVRpYsQ8Ad+C5EbBh/374ghDa 1HENzc3vBLalmXBkqp/VWH+ObcGkjbIy32BXTr4qeuon5rdTFCvgslLfUk7zI9HMlXcegc2w zGWp4Y342mAPVmNZ6UqY86ML2K41f2MGbx2VSpUBza/ZE8SgfwQqHMkcIIDcGRCdE1JcgJ6d j8uG0xjx96R6upM7zU4KF2
  • Ironport-sdr: IOkwJFIQ3Kn6ZeoXcmOr5/oyU+86gutetzvZrJ3XG7s20uOYXKsj8TpuI9E9+MtUzwF8UreioB ZeViExfhPgtal+nt+SAvgEjEr4vgVAKT+cGVLK2l3WE91a38fCP6GWyo+Vj6u+pXJhYxbBrxjl iOBsIAVXJANghcWmTZiSMXVCaBHVIodRGmBV9FjC6XFGbjhtNl5lRIOHMmhvpAqDbgJ7Ji+L3F Id1bvtuBhGUTdnqOxYDugAAdeAcZu8KcvlE33enX8kK2fg1I442PRMjvOttYHv+WrxRMF2tUbV /2d8jgPw3njOIm8R2OKVbXU8
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 01/09/2021 14:08, Jan Beulich wrote:
>>>> 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?
>> Yes.
>>>  Because that's
>>> what's getting the way here.
>> On a real machine, you really can write some executable code into an
>> E820 reserved region and jump to it.  You can also execute code from
>> real BARs is you happen to know that they are prefetchable (or you're a
>> glutton for UC reads...)
>> And there is the WPBT ACPI table which exists specifically to let
>> firmware inject drivers/applications into a windows environment, and may
>> come out of the SPI ROM in the first place.
>> Is it sensible to execute an E820 reserved region, or unmarked BAR? 
>> Probably not.
>> Should it work, because that's how real hardware behaves?  Absolutely.
>> Any restrictions beyond that want handling by some kind of introspection
>> agent which has a policy of what to do with legal-but-dodgy-looking actions.
> IOW you suggest we remove p2m_access_t parameters from various functions,
> going with just default access?

p2m_access_t was very obviously a bodge when introduced, and I doubt it
would pass today's review standards.

It is definitely a mis-design, given its ill-specified and overlapping
semantics with respect to the p2m type.

>  Of course, as pointed out in another
> reply, we'll need to split p2m_mmio_direct into multiple types then, at
> the very least to honor the potential r/o restriction of AMD IOMMU unity
> mapped regions. (FAOD all of this isn't a short term plan anyway, at least
> afaic.)

I don't think that will be necessary.

IVMDs are almost non-existent, and given how many other areas of the AMD
IOMMU spec are totally unused, I wouldn't be surprised if r/o unity
mappings were in that category too.  There's no obvious usecase for r/o,
as anything critical enough in the platform to have an IVMD in the first
place will also be non-trivial enough to require bidirectional
communication somehow.

The unity mapping only says "this device requires read-only access".  It
doesn't say "this must be mapped read-only", and it is legitimate to map
a r/o unity mapping as r/w.

If such a case actually exists, there's clearly one agent in the system
with r/w access into the r/o range, and mapping it r/w is equivalent to
the "IOMMU not enabled in the first place" case which is the default
case for most software for the past decade-and-a-bit.

In other words, I don't think the r/o unit maps on their own are a good
enough reasons to split the type.  If we gain other reasons then fine,
but this seems like chunk of complexity with no real users.




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