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

~Andrew




 


Rackspace

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