Message ID | a141e1a5-0185-4923-a91e-68c06a4f78cf@suse.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | x86/PVH: Dom0 building adjustments | expand |
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@suse.com> > --- > 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? 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. Thanks, Roger.
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@suse.com> >> --- >> 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. > 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. 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. Jan
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@suse.com> > >> --- > >> 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.
On 23.09.2021 13:54, Roger Pau Monné wrote: > 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: >>>> --- 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. As was clarified, p2m_access_t should be solely used by e.g. introspection agents, who are then the entity responsible for resolving the resulting faults. Any other uses (to e.g. merely restrict permissions for other reasons) are really abuses. That is, if e.g. a r/o direct-MMIO mapping is needed, this should not be expressed as (p2m_mmio_direct, p2m_access_r) tuple, but would require a distinct p2m_mmio_direct_ro type. >>> 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. Hmm, indeed. No deviation in either direction should be permitted. Jan
On Thu, Sep 23, 2021 at 02:15:25PM +0200, Jan Beulich wrote: > On 23.09.2021 13:54, Roger Pau Monné wrote: > > 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: > >>>> --- 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. > > As was clarified, p2m_access_t should be solely used by e.g. > introspection agents, who are then the entity responsible for > resolving the resulting faults. Any other uses (to e.g. merely > restrict permissions for other reasons) are really abuses. But some p2m types don't really have a fixed access tied to them, for example MMIO regions just inherit whatever is the default access for the domain, which makes all this look slightly weird. If the access should solely be used by introspection, then each type should have a fixed access tied to it? > That > is, if e.g. a r/o direct-MMIO mapping is needed, this should not > be expressed as (p2m_mmio_direct, p2m_access_r) tuple, but would > require a distinct p2m_mmio_direct_ro type. I guess we would then require a p2m_mmio_direct_ro, p2m_mmio_direct_rwx and a p2m_mmio_direct_n at least, and ideally we would need to differentiate the mandatory regions as present in ACPI tables using yet different types. Thanks, Roger.
On 23.09.2021 17:15, Roger Pau Monné wrote: > On Thu, Sep 23, 2021 at 02:15:25PM +0200, Jan Beulich wrote: >> On 23.09.2021 13:54, Roger Pau Monné wrote: >>> 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: >>>>>> --- 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. >> >> As was clarified, p2m_access_t should be solely used by e.g. >> introspection agents, who are then the entity responsible for >> resolving the resulting faults. Any other uses (to e.g. merely >> restrict permissions for other reasons) are really abuses. > > But some p2m types don't really have a fixed access tied to them, for > example MMIO regions just inherit whatever is the default access for > the domain, which makes all this look slightly weird. If the access > should solely be used by introspection, then each type should have a > fixed access tied to it? I think so, yes. Hence e.g. p2m_ram_ro and p2m_grant_map_r{w,o}. >> That >> is, if e.g. a r/o direct-MMIO mapping is needed, this should not >> be expressed as (p2m_mmio_direct, p2m_access_r) tuple, but would >> require a distinct p2m_mmio_direct_ro type. > > I guess we would then require a p2m_mmio_direct_ro, > p2m_mmio_direct_rwx and a p2m_mmio_direct_n at least, and ideally we > would need to differentiate the mandatory regions as present in ACPI > tables using yet different types. What would we need p2m_mmio_direct_n for? And what's the (present, not future) reason for the x in p2m_mmio_direct_rwx? Jan
On Thu, Sep 23, 2021 at 05:22:08PM +0200, Jan Beulich wrote: > On 23.09.2021 17:15, Roger Pau Monné wrote: > > On Thu, Sep 23, 2021 at 02:15:25PM +0200, Jan Beulich wrote: > >> On 23.09.2021 13:54, Roger Pau Monné wrote: > >>> 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: > >>>>>> --- 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. > >> > >> As was clarified, p2m_access_t should be solely used by e.g. > >> introspection agents, who are then the entity responsible for > >> resolving the resulting faults. Any other uses (to e.g. merely > >> restrict permissions for other reasons) are really abuses. > > > > But some p2m types don't really have a fixed access tied to them, for > > example MMIO regions just inherit whatever is the default access for > > the domain, which makes all this look slightly weird. If the access > > should solely be used by introspection, then each type should have a > > fixed access tied to it? > > I think so, yes. Hence e.g. p2m_ram_ro and p2m_grant_map_r{w,o}. > > >> That > >> is, if e.g. a r/o direct-MMIO mapping is needed, this should not > >> be expressed as (p2m_mmio_direct, p2m_access_r) tuple, but would > >> require a distinct p2m_mmio_direct_ro type. > > > > I guess we would then require a p2m_mmio_direct_ro, > > p2m_mmio_direct_rwx and a p2m_mmio_direct_n at least, and ideally we > > would need to differentiate the mandatory regions as present in ACPI > > tables using yet different types. > > What would we need p2m_mmio_direct_n for? AMD can specify no access at all for certain regions on the ACPI tables from what I've read on the manual (IW = IR = 0 in IVMD Flags). AFAICT amd_iommu_reserve_domain_unity_map can already call iommu_identity_mapping with access p2m_access_n and that would get propagated into set_identity_p2m_entry. > And what's the (present, > not future) reason for the x in p2m_mmio_direct_rwx? Mapped ROM BARs, but I'm also unsure we shouldn't just map MMIO with execute permissions by default unless stated otherwise. Thanks, Roger.
--- 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. + */ + if ( is_hardware_domain(d) && gfn_p2mt == p2m_mmio_direct && + access <= p2m_access_rwx && a <= p2m_access_rwx ) + access |= a; + if ( access == a ) { gfn_unlock(p2m, gfn, order);
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@suse.com> --- v3: Move last in series, for being controversial. v2: Split off from original patch. Accumulate all of R, W, and X.