[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: Wed, 1 Sep 2021 15:08:02 +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:X-MS-Exchange-SenderADCheck; bh=Zj3a51YS0sJYi/JgKZoY15y3HvCrl4xmlsN+RSvg7T4=; b=KaRX+6TnU6ZH7U7+J7j0O8V5NYN/X67Q5Kb3gLKgTtQ7T5gbuzl/MD9h5OUQ9tGn/U2t/g7wI1EqhK8ue6RFm46PHxtCgUF8az8/kfTkjOmlA59mh+R2hVRK6AohMZiuY2MDEwppvyWWHuYXrb42WKoAnxHM3xESEzO6LRju4ns3t3+3onTqfDOUU3MdnKDT0gQuO3TTP+B3rPwbOxSIGGiPd/muM5gOWH0ncX/w0tcLYv7EW3OCwKhyA6bmCjMRZA1R8OMnr85ylEuY5yk5HNTNsh1IUfhkXYPdSNJdynNofbBMXprqKpS5tq3GkQHjLjy2Cz/pWdshP88uv0tyBA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=SDDSdJLfPuegCUeDLRclJQFt4Tt4PDzQ9+wMnf9KuHABi1KthxETQzUrDvSbDAIrpWuean7z1HtUYAHbSOsKG7tP+grvxbqvChoQc935X92UXv/jRbZLg38VvBsW037bcayDBAiKyRagRLwqZcHcFEL3Lv40r0ytdv5qrWVDFPqYZtGE9G4f7j4iozq5vhwLIm4hWSaP5bBWhof0yNuCtUhXl4pmPmsyfdmJPrqNI/8mNRx5GH5sT99jBDAdSqYoqhQkAibdTvopdqFdYsCl3yl4WyvA+nfsfO0JcmKjY9Y1Qf4nSvGTQFOovi7xvefYT7kQsnd9gS1vk/6O2BJX/g==
  • 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: Wed, 01 Sep 2021 13:08:25 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 01.09.2021 14:47, Andrew Cooper wrote:
> 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),

I don't think your example fits: hvmloader moving RAM will first
convert the p2m slot to non-present. Then a ROM page can get mapped
there quite fine. A direct transition (without going through n/p)
would not work independent of the change here: The MFNs would
differ, as would the p2m types.

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

Well, okay, I will split the patch, despite not being convinced this
will do us any good - we'd backport just the part you consider a bug
fix, but not the part you deem a feature (and which I consider part
of the bug fix).

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

Actually I think I was missing an important aspect here: The code in
question gets used not only for PVH, but also for HVM, where pass-
through is a thing. Hence I'll restrict the "feature" part to Dom0
for now.

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




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