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

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

  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 1 Sep 2021 11:53:03 +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=AT3M3+5f9uqBf3l+DjU+HB2tK3g3eHcn74+2vK7NM2E=; b=lQyb7C5MKqEA1yrFo9gCTwm/ewjumtyAPB9SBH0TStIruQaZ+OyYE9AqXFn/+X39URQG6FouLPWbVPQ1Sv2c21YVl6lNqOwXgQ1b+ZpCyTclEOv+VU7SLwRlGQ07hp+5LgzBwNUXFp5mJWUttIZcHIwk1Ufd9A1qdwGkjUCtlZ0DRLdlg5YjXhCKJbHCwAgacQTeaOTmiO4QJ8hd579Zjb/fXZuwHr7wYagmt4QLE8DoY7Vr5VO/dYLJ63LGz23NXIpmLDQG8hMEaiP7Fw6WGqLczLnNuf+CGBnH54OGxolpQ2UYIBxgfO1F8Ldouv0+289P7Ckstf3/kTen9rcQXA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kOi5t8fkV7mE3oEPg24gICc669mhOlN2DGhKYKFInlEE5E5f3L7o2Yb+t+z7q3qvxomxmiDN6DtDXCux36Zd/3v/ErklmaWGch1hWV3GV47QBNYUu8uftl1noVPv3g+JyVx97pvYxIso4i5YxraU/lOdiCoHNXk/ZOiyqjXH7qWL5GdZJq1u+bZbLuXt/rJtM6tXz8Bb0pe+gd46r4CuUpznztHqEbfEXaVrnykhkhVmT25gM1xz5nsbLC/TzRFHrwkXMBTJJOwn4gxg0LFTo8P64BCO5hdYZm9JLA9jn6Y/n3xaUVjbysMiEeaKljmqYJHgeVeGkaYGhQr+Eaeixw==
  • 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: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 01 Sep 2021 09:53:27 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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? Insn
fetches might even trip bad hardware behavior in such a case by
being the wrong granularity. 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().

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




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