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

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

  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 7 Sep 2021 08:27:27 +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=oLQFaWi4QB5p7y9edaTmQxOxvVOecW8XZ9bI4P+cP6Q=; b=YVxfm+fb5paYsyeS1VBZUWKjRkDDM4pjVW0Pnz0G8PFshgrr6KF6CExIS/w8NuGEaH3yhsp4ty1oTJifc75RhQSJ100727l5qfv4B4vGH0iv89kuy39r4ByvkEu3OHjTGXtixqF3XvZaV5PkzpdY5Qy199nTxVW6XHn6fKgrYthW7898NC/7VOZESqXls6K9VG5gN3Z8yZgyxZKP6xuny4T8i89jntqDyl96ejartMxiIV58FJBBk8rfzpgT4vK/IZRECufg0pXe6p8MaZXNls8hky3KqWP/19RPFihdE1T8T460XmMTMtc5Fe6XKABO6XYFBQKupLwlflXsTzOCJQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JeeNiYH+ScbJodh/gqcrJpJbzdLR1nzFOS6uj5bNaf1TFAyrG7wGfjZuLzMAd8onABRPTLhZrny/En5D4v08IKWJLqNTTexCcIpzXtHd7aHfTVDYRLO+kxSpYHiMqbQpnoIyPsGey5t7mwMgCOCoonxfXrmOgBB0kXlHb+Pe0Jbl5A5E6kAnqrHhrIqSnc4aSp63EJ3jcTf4mHiPdrrx2g8Vb1Au7hkihJzWhZu0RK9k+xFiTJcnFmi3LEDC+7FSJfcPV3JYjhIjQJ6ELWY8zEYQW3Xv341H/TeyzIVXXwmUUcUoB36KEU+BTEixXpogqn5IjoUEU8SKD0dL0ukvlg==
  • 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: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 07 Sep 2021 06:27:40 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 06.09.2021 21:53, Andrew Cooper wrote:
> 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.

Well, imo that's extending what "Write permission. 1b=writeable, 0b=not
writeable" and "Read permission. 1b=readable, 0b=not readable" in the
spec say. "Permission" to me doesn't mean what you say.

Nevertheless I would perhaps not insist (as I've already made clear I
don't see a strong need to support w/o mappings), if ...

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

... there wasn't already a 2nd use for this: The IO-APIC mappings (see
my respective pending patch).




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