[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: Wed, 1 Sep 2021 13:47:40 +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:X-MS-Exchange-SenderADCheck; bh=tjDlw24xjL9N6LfcvdiORpfx0u28EjiV6lJoNjLfxq8=; b=W6o4kmppL91QG6I9Dz2uK6zVgiKL6PoXwcTmI8ViPCXT8oqW7ZoRwbIszzKSyCJj3o5ahfZCYDW+KkogUb9yWBRl1MXl0U6LVDQ4WonTHpz0GELWlCZgW30Pkk4A09ZhYZIflrLCgt2VTT7eSUfUGqJY0Z82qGqKS1KOctzpFPkhRAU9t/M/TUtPzIJRKTTE5qpNtkNgDmS7iD4RZKvWKaaxEl/lnBCV3vET1yKdWoo0aPqelgfuLKcmF29IETbYwEiAcbjL3k5t1yLaB5lMuekqc6RGGTOg4AH1mDBLrCoTUcr/SM55v2LKxlnwwZK3+Y9YbkqyWwpoKrl1rbkflw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VehwfQI3Yr0WBQFibVvQEaG1LPfYEzpwEhoCcelziCNpSFcZGwoKLwYteeQkacAKGDWgtfOK5zV9bLWt2hmH4pYbuYcAVycf0a3j2EmD0OuBzrORMcUZkgx/pISnC2J+UwR8FUnB0AuGMNU7T6Srknurfflt6SX3E/6ZXE9teW7eZAVI8h4G9jO4RJHZeYwtUAlDHf5YDgoXJIRFC1D52CMJ8lkHc0w5GjD4ftcy9k0yO10w+ySnYaVnVN+ZUjAIXjh5gigw0q0tcZH2TeNpmVlvdy9rJ+jUoGYDav2nJZ5F+7Y/jthRBBUXsjrBpPmJo+cT2r8hEbc6bIAQODioQg==
  • 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: Wed, 01 Sep 2021 12:47:53 +0000
  • Ironport-hdrordr: A9a23:O+qdP6yIvAOamiwdUOZ5KrPxouskLtp133Aq2lEZdPULSKOlfp GV8MjziyWYtN9wYhAdcdDpAtjkfZquz+8L3WB3B8bfYOCGghrUEGgG1+XfKlLbalXDH4JmpM Bdmu1FeafN5DtB/LbHCWuDYq8dKbC8mcjC74eurAYecegpUdAF0+4QMHfrLqQcfnghOXNWLu v/2iMKnUvaRZxBBf7LeEXtEtKz6OHjpdbDW1orFhQn4A6BgXeB76P7KQGR2lM7XylUybkv3G DZm0ihj5/T8s2T+1v57Sv+/p5WkNzuxp9qA9GNsNEcLnHJhhyzbIpsdrWetHQeof2p6nwtjN 7Qyi1Qcfhb2jf0RCWYsBHt0w7v3HIH7GLj80aRhT/ZrcnwVFsBeoB8rLMcViGcx1srvdl63q 4O9XmerYBrARTJmzm4z8TUVjlx/3DE40YKoKo2tThyQIEeYLheocg050VOCqoNGyr89cQODP RuNsfB//xbGGnqLEwxhlMfhOBEY05DWStvGiM5y4qoOnlt7TBEJnIjtYkidixqzuNld3EsjN 60QZiBl9l1P4QrhOxGdb88qWbeMB26ffv2ChPnHb3QLtBOB5v8ke+D3FwL3pDcRHUp9up+pH 2TaiIViYYNE3ieQPFmmqc7qSzwfA==
  • Ironport-sdr: yt6rC4h0IVg9WGAgQqBw+QI6Vyb4RHsSc6ZlVB+K0T/55JP6s6qj35S9ggo30I++PdE4fBkTFr UD2cYkH3DbnCECHRq9hRLxSqhqw+WvMyxBId3EmoXf/QlWSfIQkbJJtt2CefiqLayQ01zWdrDS zFst2jTkHAdQslKjesrQKNk2uN2UOdPy9OTTpBTkWHGX5BKcVDD8p5dlO5nr6X5mi/6OZ4Hpzb shOmxOSp/r0AIyJffE4nRU/dzjSRC59fxXjoSzZliKPubAs1ivOp3/cjr3Ggb0M+vIzTDqgg1r 7sHXvMP44Xpog+lgvMoVYiW3
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 31/08/2021 16:38, 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.

But you're conflating a feature (under question anyway, because I gave
you an example where I expect this will collide in a regular domU
already), with an emergency bugfix to unbreak staging caused by an
unexpected interaction in a security hotfix.

At an absolute minimum, this patch needs splitting in two to separate
the bugfix from the proposed feature.

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

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.

> Plus, as a side note, then we don't even have e.g. IOMMUF_executable.

Just as I vs D side reads are indistinguishable outside of the CPU core,
the same is in principle true for PCI devices which execute code (e.g.
GPU shaders).  Reads on the bus are just reads.

That said, the latest SIOV spec does appear to include the ER (Execution
Request) bit for use with PASSID/Shared-Virtual-Memory, which interacts
with the EPT-X/ia32-NX bits, and goes as far as having SMEP/SMAP bits in
the IOMMU configuration.  I'm not sure if this is in released hardware
yet, but it's clearly on the horizon.  I can't spot any execute related
controls in the AMD IOMMU spec, although it does have user/supervisor




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