| Message ID | 20221121143731.27545-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Even more XSA-409 fixes | expand |
Hi Andrew, > -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: Monday, November 21, 2022 10:38 PM > To: Xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Daniel De Graaf > <dgdegra@tycho.nsa.gov>; Daniel Smith <dpsmith@apertussolutions.com>; > Jason Andryuk <jandryuk@gmail.com>; Henry Wang > <Henry.Wang@arm.com> > Subject: [PATCH 2/2] xen/flask: Wire up > XEN_DOMCTL_{get,set}_paging_mempool_size > > These were overlooked in the original patch, and noticed by OSSTest which > does > run some Flask tests. > > Fixes: 22b20bd98c02 ("xen: Introduce non-broken hypercalls for the paging > mempool size") > Suggested-by: Daniel Smith <dpsmith@apertussolutions.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
On Mon, Nov 21, 2022 at 9:37 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > These were overlooked in the original patch, and noticed by OSSTest which does > run some Flask tests. > > Fixes: 22b20bd98c02 ("xen: Introduce non-broken hypercalls for the paging mempool size") > Suggested-by: Daniel Smith <dpsmith@apertussolutions.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> > CC: Daniel Smith <dpsmith@apertussolutions.com> > CC: Jason Andryuk <jandryuk@gmail.com> > CC: Henry Wang <Henry.Wang@arm.com> Reviewed-by: Jason Andryuk <jandryuk@gmail.com> Thanks, Andrew. Though we might want a small tweak - possibly as a follow up? > diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if > index 424daab6a022..6b7b7d403ab4 100644 > --- a/tools/flask/policy/modules/xen.if > +++ b/tools/flask/policy/modules/xen.if > @@ -92,7 +92,7 @@ define(`manage_domain', ` > allow $1 $2:domain { getdomaininfo getvcpuinfo getaffinity > getaddrsize pause unpause trigger shutdown destroy > setaffinity setdomainmaxmem getscheduler resume > - setpodtarget getpodtarget }; > + setpodtarget getpodtarget getpagingmempool setpagingmempool }; There is also create_domain_common which is for a dedicated "domain builder" that creates but does not manage domains. I think that should gain setpagingmempool permission? Regards, Jason
On 21/11/2022 15:39, Jason Andryuk wrote: > On Mon, Nov 21, 2022 at 9:37 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> These were overlooked in the original patch, and noticed by OSSTest which does >> run some Flask tests. >> >> Fixes: 22b20bd98c02 ("xen: Introduce non-broken hypercalls for the paging mempool size") >> Suggested-by: Daniel Smith <dpsmith@apertussolutions.com> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> CC: Daniel Smith <dpsmith@apertussolutions.com> >> CC: Jason Andryuk <jandryuk@gmail.com> >> CC: Henry Wang <Henry.Wang@arm.com> > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> > > Thanks, Andrew. Though we might want a small tweak - possibly as a follow up? > >> diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if >> index 424daab6a022..6b7b7d403ab4 100644 >> --- a/tools/flask/policy/modules/xen.if >> +++ b/tools/flask/policy/modules/xen.if >> @@ -92,7 +92,7 @@ define(`manage_domain', ` >> allow $1 $2:domain { getdomaininfo getvcpuinfo getaffinity >> getaddrsize pause unpause trigger shutdown destroy >> setaffinity setdomainmaxmem getscheduler resume >> - setpodtarget getpodtarget }; >> + setpodtarget getpodtarget getpagingmempool setpagingmempool }; > There is also create_domain_common which is for a dedicated "domain > builder" that creates but does not manage domains. I think that > should gain setpagingmempool permission? Sounds like it should. Something like this? diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if index 6b7b7d403ab4..11c1562aa5da 100644 --- a/tools/flask/policy/modules/xen.if +++ b/tools/flask/policy/modules/xen.if @@ -49,7 +49,8 @@ define(`create_domain_common', ` allow $1 $2:domain { create max_vcpus setdomainmaxmem setaddrsize getdomaininfo hypercall setvcpucontext getscheduler getvcpuinfo getaddrsize getaffinity setaffinity - settime setdomainhandle getvcpucontext set_misc_info }; + settime setdomainhandle getvcpucontext set_misc_info + getpagingmempool setpagingmempool }; allow $1 $2:domain2 { set_cpu_policy settsc setscheduler setclaim set_vnumainfo get_vnumainfo cacheflush psr_cmt_op psr_alloc soft_reset I can fold this in on commit. ~Andrew
On Mon, Nov 21, 2022 at 10:46 AM Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 21/11/2022 15:39, Jason Andryuk wrote: > > On Mon, Nov 21, 2022 at 9:37 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> These were overlooked in the original patch, and noticed by OSSTest which does > >> run some Flask tests. > >> > >> Fixes: 22b20bd98c02 ("xen: Introduce non-broken hypercalls for the paging mempool size") > >> Suggested-by: Daniel Smith <dpsmith@apertussolutions.com> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> --- > >> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> > >> CC: Daniel Smith <dpsmith@apertussolutions.com> > >> CC: Jason Andryuk <jandryuk@gmail.com> > >> CC: Henry Wang <Henry.Wang@arm.com> > > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> > > > > Thanks, Andrew. Though we might want a small tweak - possibly as a follow up? > > > >> diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if > >> index 424daab6a022..6b7b7d403ab4 100644 > >> --- a/tools/flask/policy/modules/xen.if > >> +++ b/tools/flask/policy/modules/xen.if > >> @@ -92,7 +92,7 @@ define(`manage_domain', ` > >> allow $1 $2:domain { getdomaininfo getvcpuinfo getaffinity > >> getaddrsize pause unpause trigger shutdown destroy > >> setaffinity setdomainmaxmem getscheduler resume > >> - setpodtarget getpodtarget }; > >> + setpodtarget getpodtarget getpagingmempool setpagingmempool }; > > There is also create_domain_common which is for a dedicated "domain > > builder" that creates but does not manage domains. I think that > > should gain setpagingmempool permission? > > Sounds like it should. Something like this? > > diff --git a/tools/flask/policy/modules/xen.if > b/tools/flask/policy/modules/xen.if > index 6b7b7d403ab4..11c1562aa5da 100644 > --- a/tools/flask/policy/modules/xen.if > +++ b/tools/flask/policy/modules/xen.if > @@ -49,7 +49,8 @@ define(`create_domain_common', ` > allow $1 $2:domain { create max_vcpus setdomainmaxmem setaddrsize > getdomaininfo hypercall setvcpucontext getscheduler > getvcpuinfo getaddrsize getaffinity setaffinity > - settime setdomainhandle getvcpucontext > set_misc_info }; > + settime setdomainhandle getvcpucontext set_misc_info > + getpagingmempool setpagingmempool }; > allow $1 $2:domain2 { set_cpu_policy settsc setscheduler setclaim > set_vnumainfo get_vnumainfo cacheflush > psr_cmt_op psr_alloc soft_reset > > I can fold this in on commit. Yes, though strictly speaking it may only need setpagingmempool and not getpagingmempool. These are all calls that would execute before the domain is run. But adding both is probably fine since the builder is setting these itself. i.e. allowing it to read what it set is fine. So, yes, I'd just fold this in. Thanks, Jason
On 11/21/22 09:37, Andrew Cooper wrote: > These were overlooked in the original patch, and noticed by OSSTest which does > run some Flask tests. > > Fixes: 22b20bd98c02 ("xen: Introduce non-broken hypercalls for the paging mempool size") > Suggested-by: Daniel Smith <dpsmith@apertussolutions.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> > CC: Daniel Smith <dpsmith@apertussolutions.com> > CC: Jason Andryuk <jandryuk@gmail.com> > CC: Henry Wang <Henry.Wang@arm.com> > > It should be noted that the original XSA-409 fix broke Flask on ARM but no > testing noticed. c/s 7c3bbd940dd8 ("xen/arm, libxl: Revert > XEN_DOMCTL_shadow_op; use p2m mempool hypercalls") "fixes" the original > breakage and introduced this breakage instead. > > For 4.17. It's a fix for an issue that OSSTest is currently blocking as a > regression. Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te index f710ff9941c0..f1dcff48e227 100644 --- a/tools/flask/policy/modules/dom0.te +++ b/tools/flask/policy/modules/dom0.te @@ -35,7 +35,8 @@ allow dom0_t dom0_t:domain { setvcpucontext max_vcpus setaffinity getaffinity getscheduler getdomaininfo getvcpuinfo getvcpucontext setdomainmaxmem setdomainhandle setdebugging hypercall settime setaddrsize getaddrsize trigger - getpodtarget setpodtarget set_misc_info set_virq_handler + getpodtarget setpodtarget getpagingmempool setpagingmempool set_misc_info + set_virq_handler }; allow dom0_t dom0_t:domain2 { set_cpu_policy gettsc settsc setscheduler set_vnumainfo diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if index 424daab6a022..6b7b7d403ab4 100644 --- a/tools/flask/policy/modules/xen.if +++ b/tools/flask/policy/modules/xen.if @@ -92,7 +92,7 @@ define(`manage_domain', ` allow $1 $2:domain { getdomaininfo getvcpuinfo getaffinity getaddrsize pause unpause trigger shutdown destroy setaffinity setdomainmaxmem getscheduler resume - setpodtarget getpodtarget }; + setpodtarget getpodtarget getpagingmempool setpagingmempool }; allow $1 $2:domain2 set_vnumainfo; ') diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 391aec4dc221..78225f68c15c 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -822,6 +822,12 @@ static int cf_check flask_domctl(struct domain *d, int cmd) case XEN_DOMCTL_get_cpu_policy: return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GET_CPU_POLICY); + case XEN_DOMCTL_get_paging_mempool_size: + return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__GETPAGINGMEMPOOL); + + case XEN_DOMCTL_set_paging_mempool_size: + return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__SETPAGINGMEMPOOL); + default: return avc_unknown_permission("domctl", cmd); } diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index 6359c7fc8757..4e6710a63e1b 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -180,6 +180,10 @@ class domain set_misc_info # XEN_DOMCTL_set_virq_handler set_virq_handler +# XEN_DOMCTL_get_paging_mempool_size + getpagingmempool +# XEN_DOMCTL_set_paging_mempool_size + setpagingmempool } # This is a continuation of class domain, since only 32 permissions can be
These were overlooked in the original patch, and noticed by OSSTest which does run some Flask tests. Fixes: 22b20bd98c02 ("xen: Introduce non-broken hypercalls for the paging mempool size") Suggested-by: Daniel Smith <dpsmith@apertussolutions.com> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> CC: Daniel Smith <dpsmith@apertussolutions.com> CC: Jason Andryuk <jandryuk@gmail.com> CC: Henry Wang <Henry.Wang@arm.com> It should be noted that the original XSA-409 fix broke Flask on ARM but no testing noticed. c/s 7c3bbd940dd8 ("xen/arm, libxl: Revert XEN_DOMCTL_shadow_op; use p2m mempool hypercalls") "fixes" the original breakage and introduced this breakage instead. For 4.17. It's a fix for an issue that OSSTest is currently blocking as a regression. --- tools/flask/policy/modules/dom0.te | 3 ++- tools/flask/policy/modules/xen.if | 2 +- xen/xsm/flask/hooks.c | 6 ++++++ xen/xsm/flask/policy/access_vectors | 4 ++++ 4 files changed, 13 insertions(+), 2 deletions(-)