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

Re: [PATCH v3 9/9] x86/P2M: relax permissions of PVH Dom0's MMIO entries


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 23 Sep 2021 13:54:59 +0200
  • 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=zl8HNQkhaNkXceTYB3Jh85Oznx7vvJQEKqJc+/lXEcQ=; b=ETeA78JNUvg+r2FtXn0B0OrRLxoaw7jzsuxTRYh7Ah+hoCieHJ4it2PyvdarpK2ybtsZQNrdmD1HokuBDynxQyEOhqnbjiitw9G989x4zFkjLMvzD+FCapBY16Ycg9sz0nS7u9jZ/xgfhk1soMoFGHUZH59QiVjFDnO9D7aPanwLk/M8HAcFZCYLpvZuOPpDuLgbl2EbxJggn/O7lJMJmKuiEFpOt5gh7G50m2xHoEigzsDnBryUNKQdV7WfMPZ4WZyQA/lY8fhEqCSAzp4S9QRMq3Bt1L1QYPui4vBh1tIqnm7PRda9uK/6saPZRqIaT8MHCTbXELGRPyygjfbu+w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JzQKPyq3Y2MMXbsZ54ZbP9TXwmgmCRkvBmECFvNsw5RLepTzHoADFzoVgEC6/CkHW/oC0+FPj/7f6uroD1sFH2n5gLtgCG0eQ90fiXl4/GJA+jaqKyOY+5EzxRCZh6ZnDhExeE7GPA5HsCquhz26rY6t8Q1O+iMDq1Acanl5l/ulfBa1LK6hxMcIL8GlTN4sreiWBUPLHxN3i/vnpN0ifULfr8hChZkr9p3edp4CpiD+vY+6qtDTcbSazwQRI2fhsbDAKrskj1FBiBcYR4pBd4g9xkOH9YLAV8SW8dYkZibGWmUBFVC2HEerE9A5gxBF50/L/9iyHSyaNgy3J1gOzQ==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Andrew Cooper" <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>
  • Delivery-date: Thu, 23 Sep 2021 11:55:15 +0000
  • Ironport-data: A9a23:ACmKoa/sAnMpE7oXZ1ceDrUDXnmTJUtcMsCJ2f8bNWPcYEJGY0x3m mpOXDjQb/bYa2T2ct0iYYm3/RlV68SGy9MyT1Nq/Hg8E34SpcT7XtnIdU2Y0wF+jyHgoOCLy +1EN7Es+ehtFie0Si9AttENlFEkvU2ybuOU5NXsZ2YhGGeIdA970Ug6w79j2dYx6TSEK1jlV e3a8pW31GCNg1aYAkpMg05UgEoy1BhakGpwUm0WPZinjneH/5UmJMt3yZWKB2n5WuFp8tuSH I4v+l0bElTxpH/BAvv9+lryn9ZjrrT6ZWBigVIOM0Sub4QrSoXfHc/XOdJFAXq7hQllkPhRw vkSj52fSDs2ZI3phOUiQyV5DR1XaPguFL/veRBTsOSWxkzCNXDt3+9vHAc9OohwFuRfWD8Us 6ZCcXZUM07F17neLLGTE4GAguw5K8bmJsUHs2xIxjDFF/c2B5vERs0m4PcFh2ts1pkfQJ4yY eJBbTpxZgT7XidsPwkvJJY1s92XvDrGJmgwRFW9+vNsvjm7IBZK+KfpGMrYfJqNX8o9tlaVo CfK8nr0BjkeNceD0nyV/3S0nOjNkCjnHoUIG9WQ9PRnnVmSzWw7EwANWB2wpvzRt6Klc4sBc QpOoHNo9PVsshzwJjXgY/GmiF6OkkEmWvVRL/JgyCSRy5XE3yeHOkFRG1atd+canMMxQDUr0 HqAkNXoGSFjvdWpdJ6NyluHhWjtYnVPcwfucQdBFFFcsoS/+enfmzqSFo4LLUKjsjHi9dgcK RixpS4ijv04iccR3s1XFniW3mrx/vAlouMzjzg7v15JDCslP+ZJhKTysDA3CMqsyq7DFTFtW 1Bex6CjABgmV83lqcB0aLxl8EuVz/iEKibAplVkAoMs8T+gk1b6I9sKu2omfBw3aJdVEdMMX KM1kVkKjHO0FCH2BZKbnqrrU5h6pUQePY6Nug/ogipmPcEqKV7vENBGbk+MxWH9+HXAYollU ap3hf2EVC5AYYw+lWLeb75EjdcDm3BvrUuOFMuT50n2jtKjiIu9FO5t3K2mNbtisstpYWz9r r5iCid9408EDbKiMnWJq957wJJjBSFTOK0aYvd/L4arCgFnBHsgG7nWx7YgcJZihKNbiqHD+ XTVZ6OS4AGXaaTvJVrYZ3Z9RqnoWJoj/3s3MTZ1ZQSj2mQ5YJbp56AaLsNlcb4i/e1l7Ph1U /haJJnQXqUREmzKq2YHcJ3wjI1+bxD31wiACDWoPWokdJl6Sg2XptK9Jlnz9DMDBzacvNclp +HyzRvSRJcOHlwwDMvfZP+14Um2uHwRxLB7U0fSe4EBc0Tw6ol6bSf2i6Zvcc0LLBzCwBqc1 hqXXkhE9bWc/ddt/YCQ166eroqvH+9vJWZgHjHWveSsKC3X3mu/2oscAuyGSi/QCTHv86K4a OQLk/ylaK8bnExHupZXGqpwyf5s/MPmorJXw1g2HHjPaFj3WLpsLmPfgJtKv6xJgLRYpRG3S gSE/dwDYeeFP8bsEVgwIgs5b7vciaFIy2eKtfllcl/n4CJX/aacVRQANhaBvyVRMb9pPd532 uwmosMXt1SyhxdC3gxqVcyIG7Bg9kA9bpg=
  • Ironport-hdrordr: A9a23:jh29qa7TpENJ1D07NwPXwVuBI+orL9Y04lQ7vn2ZFiY6TiXIra +TdaoguSMc6AxwZJkh8erwXpVoZUmsiKKdhrNhQYtKPTOWwldASbsC0WKM+UyEJ8STzJ846U 4kSdkANDSSNykLsS+Z2njBLz9I+rDum8rE9ISurQYfcegpUdAa0+4QMHfrLqQcfng+OXNWLu v62iIRzADQB0j/I/7LSkUtbqzmnZnmhZjmaRkJC1oO7xSPtyqh7PrfHwKD1hkTfjtTyfN6mF K13TDR1+GGibWW2xXc32jc49B/n8bg8MJKAIiphtIOIjvhpw60bMBKWqGEvhoyvOazgWxa3+ XkklMFBYBe+nnRdma6rV/E3BTh6i8n7zvYxVqRkRLY0IfEbQN/L/AEqZNScxPf5UZllsp7yr h302WQsIcSJQ/cnQzmjuK4FC1Cpw6Rmz4PgOQTh3tQXc81c7lKt7ES+0tTDdMpAD/60oY6C+ NjZfuspMq+SWnqKkwxg1MfhOBFBh8Ib1C7qwk5y42oOgFt7TJEJxBy/r1Yop8CnKhNA6Wsqd 60a5iBOdl1P7grhJlGdZI8qP2MeyXwqCL3QRCvyGvcZdU60lL22tTKCeYOlayXkKJh9upFpH 2GaiIBiVIP
  • Ironport-sdr: NcP5u+Bz3F64K0KPa/n4y+XV5ihyRP/qO/AReLsDExZ/vYED7CCS79+tG9qgZbuMPdMuz5oc3d iYrEoqzClfW5TjburCWnxgXqN55CtCBMnhl/tvz7hgF1cIbdjVTr8Iqi6OOsqnQyYIu//6TsWt hjXwZCWOsJ7PU880vWy9qqFdziNwG1eV6pKdG3lG0OPZVaVmVaRxEzUTy8U0jOuEIkecPpoMYt PjWak4RllCFpLmtr6HeJM2x5UTWekBQarbqCWWoB4WiZsBiZDsLCaDJ528Od8AjfzoSE2f4w+H 4Y4tcpFacbKSkXaDsUsNowy9
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Sep 23, 2021 at 01:32:52PM +0200, Jan Beulich wrote:
> On 23.09.2021 13:10, Roger Pau Monné wrote:
> > On Tue, Sep 21, 2021 at 09:21:11AM +0200, Jan Beulich wrote:
> >> To become independent of the sequence of mapping operations, permit
> >> "access" to accumulate for Dom0, noting that there's not going to be an
> >> introspection agent for it which this might interfere with. While e.g.
> >> ideally only ROM regions would get mapped with X set, getting there is
> >> quite a bit of work. Plus the use of p2m_access_* here is abusive in the
> >> first place.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> v3: Move last in series, for being controversial.
> >> v2: Split off from original patch. Accumulate all of R, W, and X.
> >>
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -1319,6 +1319,18 @@ static int set_typed_p2m_entry(struct do
> >>              return -EPERM;
> >>          }
> >>  
> >> +        /*
> >> +         * Gross bodge, to go away again rather sooner than later:
> >> +         *
> >> +         * For MMIO allow access permissions to accumulate, but only for 
> >> Dom0.
> >> +         * Since set_identity_p2m_entry() and set_mmio_p2m_entry() differ 
> >> in
> >> +         * the way they specify "access", this will allow the ultimate 
> >> result
> >> +         * to be independent of the sequence of operations.
> > 
> > Wouldn't it be better to 'fix' those operations so that they can work
> > together?
> 
> Yes, but then we should do this properly by removing all abuse of
> p2m_access_t.

I'm not sure I follow what that abuse is.

> > It's my understanding that set_identity_p2m_entry is the one that has
> > strong requirements regarding the access permissions, as on AMD ACPI
> > tables can specify how should regions be mapped.
> > 
> > A possible solution might be to make set_mmio_p2m_entry more tolerant
> > to how present mappings are handled. For once that function doesn't
> > let callers specify access permissions, so I would consider that if a
> > mapping is present on the gfn and it already points to the requested
> > mfn no error should be returned to the caller. At the end the 'default
> > access' for that gfn -> mfn relation is the one established by
> > set_identity_p2m_entry and shouldn't be subject to the p2m default
> > access.
> 
> I think further reducing access is in theory supposed to be possible.

Couldn't an access reduction introduce issues, kind of similar to what
would happen if the regions are unmapped? (and that XSA-378 addressed)

I think whatever permissions set_identity_p2m_entry sets should be
mandatory ones, and no changes should be allowed. That would maybe
require introducing a new p2m type (p2m_mmio_mandatory) in order to
differentiate firmware mandatory MMIO mappings from the rest.

> For Dom0 all of this (including the potential of default access not
> being RWX) a questionable thing though, as pointed out in earlier
> discussions. After all there's no introspection (or alike) agent
> supposed to be controlling Dom0.

Ideally I would prefer a solution that could be applied to both dom0
and domU, if that's possible.

Thanks, Roger.



 


Rackspace

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