| Message ID | 20210922082123.54374-7-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gnttab: add per-domain controls | expand |
Hi Roger, On 22/09/2021 13:21, Roger Pau Monne wrote: > Allow setting max_grant_version to 0 in order to disable grant table > usage by a domain. This prevents allocating the grant-table structure > inside of Xen and requires guards to be added in several functions in > order to prevent dereferencing the structure. > > Note that a domain without a grant table could still use some of the > grant related hypercalls, it could for example issue a GNTTABOP_copy > of a grant reference from a remote domain into a local frame. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > docs/man/xl.cfg.5.pod.in | 4 +- > tools/libs/light/libxl_dom.c | 2 +- > xen/common/grant_table.c | 100 +++++++++++++++++++++++++++++++++-- > 3 files changed, 98 insertions(+), 8 deletions(-) > > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in > index c5a447dfcd..d507540c2c 100644 > --- a/docs/man/xl.cfg.5.pod.in > +++ b/docs/man/xl.cfg.5.pod.in > @@ -583,8 +583,8 @@ L<xl.conf(5)>. > =item B<max_grant_version=NUMBER> > > Specify the maximum grant table version the domain is allowed to use. Current > -supported versions are 1 and 2. The default value is settable via > -L<xl.conf(5)>. > +supported versions are 1 and 2. Setting to 0 disables the grant table for the > +domain. The default value is settable via L<xl.conf(5)>. Technically, the version only applies to format of the table for granting page. The mapping itself is version agnostic. So this feels a bit wrong to use max_grant_version to not allocate d->grant_table. I also can see use-cases where we may want to allow a domain to grant page but not map grant (for instance, a further hardening of XSA-380). Therefore, I think we want to keep max_grant_version for the table itself and manage the mappings separately (possibly by letting the admin to select the max number of entries in the maptrack). Cheers,
On 22.09.21 11:19, Julien Grall wrote: > Hi Roger, > > On 22/09/2021 13:21, Roger Pau Monne wrote: >> Allow setting max_grant_version to 0 in order to disable grant table >> usage by a domain. This prevents allocating the grant-table structure >> inside of Xen and requires guards to be added in several functions in >> order to prevent dereferencing the structure. >> >> Note that a domain without a grant table could still use some of the >> grant related hypercalls, it could for example issue a GNTTABOP_copy >> of a grant reference from a remote domain into a local frame. >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> docs/man/xl.cfg.5.pod.in | 4 +- >> tools/libs/light/libxl_dom.c | 2 +- >> xen/common/grant_table.c | 100 +++++++++++++++++++++++++++++++++-- >> 3 files changed, 98 insertions(+), 8 deletions(-) >> >> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in >> index c5a447dfcd..d507540c2c 100644 >> --- a/docs/man/xl.cfg.5.pod.in >> +++ b/docs/man/xl.cfg.5.pod.in >> @@ -583,8 +583,8 @@ L<xl.conf(5)>. >> =item B<max_grant_version=NUMBER> >> Specify the maximum grant table version the domain is allowed to >> use. Current >> -supported versions are 1 and 2. The default value is settable via >> -L<xl.conf(5)>. >> +supported versions are 1 and 2. Setting to 0 disables the grant table >> for the >> +domain. The default value is settable via L<xl.conf(5)>. > > Technically, the version only applies to format of the table for > granting page. The mapping itself is version agnostic. So this feels a > bit wrong to use max_grant_version to not allocate d->grant_table. > > I also can see use-cases where we may want to allow a domain to grant > page but not map grant (for instance, a further hardening of XSA-380). > Therefore, I think we want to keep max_grant_version for the table > itself and manage the mappings separately (possibly by letting the admin > to select the max number of entries in the maptrack). You mean the already existing domain config option max_maptrack_frames? Juergen
Hi Juergen, On 22/09/2021 14:38, Juergen Gross wrote: > On 22.09.21 11:19, Julien Grall wrote: >> Hi Roger, >> >> On 22/09/2021 13:21, Roger Pau Monne wrote: >>> Allow setting max_grant_version to 0 in order to disable grant table >>> usage by a domain. This prevents allocating the grant-table structure >>> inside of Xen and requires guards to be added in several functions in >>> order to prevent dereferencing the structure. >>> >>> Note that a domain without a grant table could still use some of the >>> grant related hypercalls, it could for example issue a GNTTABOP_copy >>> of a grant reference from a remote domain into a local frame. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> docs/man/xl.cfg.5.pod.in | 4 +- >>> tools/libs/light/libxl_dom.c | 2 +- >>> xen/common/grant_table.c | 100 +++++++++++++++++++++++++++++++++-- >>> 3 files changed, 98 insertions(+), 8 deletions(-) >>> >>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in >>> index c5a447dfcd..d507540c2c 100644 >>> --- a/docs/man/xl.cfg.5.pod.in >>> +++ b/docs/man/xl.cfg.5.pod.in >>> @@ -583,8 +583,8 @@ L<xl.conf(5)>. >>> =item B<max_grant_version=NUMBER> >>> Specify the maximum grant table version the domain is allowed to >>> use. Current >>> -supported versions are 1 and 2. The default value is settable via >>> -L<xl.conf(5)>. >>> +supported versions are 1 and 2. Setting to 0 disables the grant >>> table for the >>> +domain. The default value is settable via L<xl.conf(5)>. >> >> Technically, the version only applies to format of the table for >> granting page. The mapping itself is version agnostic. So this feels a >> bit wrong to use max_grant_version to not allocate d->grant_table. >> >> I also can see use-cases where we may want to allow a domain to grant >> page but not map grant (for instance, a further hardening of XSA-380). >> Therefore, I think we want to keep max_grant_version for the table >> itself and manage the mappings separately (possibly by letting the >> admin to select the max number of entries in the maptrack). > > You mean the already existing domain config option max_maptrack_frames? Yes. I didn't realize we already had an option for that :). Cheers,
On 22.09.2021 11:19, Julien Grall wrote: > On 22/09/2021 13:21, Roger Pau Monne wrote: >> --- a/docs/man/xl.cfg.5.pod.in >> +++ b/docs/man/xl.cfg.5.pod.in >> @@ -583,8 +583,8 @@ L<xl.conf(5)>. >> =item B<max_grant_version=NUMBER> >> >> Specify the maximum grant table version the domain is allowed to use. Current >> -supported versions are 1 and 2. The default value is settable via >> -L<xl.conf(5)>. >> +supported versions are 1 and 2. Setting to 0 disables the grant table for the >> +domain. The default value is settable via L<xl.conf(5)>. > > Technically, the version only applies to format of the table for > granting page. The mapping itself is version agnostic. So this feels a > bit wrong to use max_grant_version to not allocate d->grant_table. > > I also can see use-cases where we may want to allow a domain to grant > page but not map grant (for instance, a further hardening of XSA-380). Or the other way around: A typical Dom0 may not have a need to grant anything, but will likely want to be able to map grants. Nevertheless I think an overall "no grant operations at all" switch is good; both of the sub-aspects already have controls. Jan
On 22.09.2021 10:21, Roger Pau Monne wrote: > Allow setting max_grant_version to 0 in order to disable grant table > usage by a domain. This prevents allocating the grant-table structure > inside of Xen and requires guards to be added in several functions in > order to prevent dereferencing the structure. > > Note that a domain without a grant table could still use some of the > grant related hypercalls, it could for example issue a GNTTABOP_copy > of a grant reference from a remote domain into a local frame. I guess I'd consider this wrong - no grant table should imo mean no grant operations at all. Disabling granting can be done by setting the frame count to zero, while disabling the mapping of grants can be done by forcing no maptrack table. That way the number of places where checks need adding would reduce quite a bit. > @@ -1037,6 +1043,14 @@ map_grant_ref( > } > > rgt = rd->grant_table; > + if ( !rgt ) > + { > + put_maptrack_handle(lgt, handle); > + rcu_unlock_domain(rd); > + gdprintk(XENLOG_INFO, "%pd has no grant table\n", rd); > + op->status = GNTST_bad_domain; > + return; I would pull this check earlier, to simplify error cleanup. It could live right after having established rd. > @@ -1367,6 +1381,13 @@ unmap_common( > ld = current->domain; > lgt = ld->grant_table; > > + if ( !lgt ) > + { > + gdprintk(XENLOG_INFO, "%pd has no grant table\n", ld); > + op->status = GNTST_bad_domain; > + return; > + } While this is necessary, ... > @@ -1406,6 +1427,13 @@ unmap_common( > TRACE_1D(TRC_MEM_PAGE_GRANT_UNMAP, dom); > > rgt = rd->grant_table; > + if ( !rgt ) > + { > + rcu_unlock_domain(rd); > + gdprintk(XENLOG_INFO, "%pd has no grant table\n", rd); > + op->status = GNTST_bad_domain; > + return; > + } .. this looks to simply be a bug check, i.e. may want to be BUG_ON(). There's can't be anything to unmap if the mapping of a grant of that domain can't have succeeded. > @@ -1556,6 +1584,12 @@ unmap_common_complete(struct gnttab_unmap_common *op) > > rcu_lock_domain(rd); > rgt = rd->grant_table; > + if ( !rgt ) > + { > + rcu_unlock_domain(rd); > + op->status = GNTST_bad_domain; > + return; > + } Same here, I think. > @@ -2138,6 +2174,11 @@ gnttab_query_size( > } > > gt = d->grant_table; > + if ( !gt ) > + { > + op.status = GNTST_bad_domain; > + goto out; > + } I'm not sure here - I could also see this report zero (and success). > @@ -3270,6 +3327,11 @@ gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop, > } > > gt = d->grant_table; > + if ( !gt ) > + { > + op.status = GNTST_bad_domain; > + goto out2; > + } While not simplifying error cleanup here, I think this might still benefit from getting moved ahead of the XSM hook. There's no point querying XSM in this case. Jan
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index c5a447dfcd..d507540c2c 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -583,8 +583,8 @@ L<xl.conf(5)>. =item B<max_grant_version=NUMBER> Specify the maximum grant table version the domain is allowed to use. Current -supported versions are 1 and 2. The default value is settable via -L<xl.conf(5)>. +supported versions are 1 and 2. Setting to 0 disables the grant table for the +domain. The default value is settable via L<xl.conf(5)>. =item B<transitive_grants=BOOLEAN> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c index e9f58ee4b2..afc8b88497 100644 --- a/tools/libs/light/libxl_dom.c +++ b/tools/libs/light/libxl_dom.c @@ -598,7 +598,7 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid, LOGE(ERROR, "xc_dom_boot_image failed"); goto out; } - if ( (ret = xc_dom_gnttab_init(dom)) != 0 ) { + if ( info->max_grant_version && (ret = xc_dom_gnttab_init(dom)) != 0 ) { LOGE(ERROR, "xc_dom_gnttab_init failed"); goto out; } diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index f50e9f6a06..df01d03ce4 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -1027,6 +1027,12 @@ map_grant_ref( } lgt = ld->grant_table; + if ( !lgt ) + { + gdprintk(XENLOG_INFO, "%pd has no grant table\n", ld); + op->status = GNTST_bad_domain; + return; + } handle = get_maptrack_handle(lgt); if ( unlikely(handle == INVALID_MAPTRACK_HANDLE) ) { @@ -1037,6 +1043,14 @@ map_grant_ref( } rgt = rd->grant_table; + if ( !rgt ) + { + put_maptrack_handle(lgt, handle); + rcu_unlock_domain(rd); + gdprintk(XENLOG_INFO, "%pd has no grant table\n", rd); + op->status = GNTST_bad_domain; + return; + } grant_read_lock(rgt); /* Bounds check on the grant ref */ @@ -1367,6 +1381,13 @@ unmap_common( ld = current->domain; lgt = ld->grant_table; + if ( !lgt ) + { + gdprintk(XENLOG_INFO, "%pd has no grant table\n", ld); + op->status = GNTST_bad_domain; + return; + } + if ( unlikely(op->handle >= lgt->maptrack_limit) ) { gdprintk(XENLOG_INFO, "Bad d%d handle %#x\n", @@ -1406,6 +1427,13 @@ unmap_common( TRACE_1D(TRC_MEM_PAGE_GRANT_UNMAP, dom); rgt = rd->grant_table; + if ( !rgt ) + { + rcu_unlock_domain(rd); + gdprintk(XENLOG_INFO, "%pd has no grant table\n", rd); + op->status = GNTST_bad_domain; + return; + } grant_read_lock(rgt); @@ -1556,6 +1584,12 @@ unmap_common_complete(struct gnttab_unmap_common *op) rcu_lock_domain(rd); rgt = rd->grant_table; + if ( !rgt ) + { + rcu_unlock_domain(rd); + op->status = GNTST_bad_domain; + return; + } grant_read_lock(rgt); @@ -1931,10 +1965,7 @@ int grant_table_init(struct domain *d, int max_grant_frames, if ( max_grant_version == XEN_DOMCTL_GRANT_version_default ) max_grant_version = opt_gnttab_max_version; if ( !max_grant_version ) - { - dprintk(XENLOG_INFO, "Invalid grant table version 0 requested\n"); - return -EINVAL; - } + return 0; if ( max_grant_version > opt_gnttab_max_version ) { dprintk(XENLOG_INFO, @@ -2056,6 +2087,11 @@ gnttab_setup_table( } gt = d->grant_table; + if ( !gt ) + { + op.status = GNTST_bad_domain; + goto out; + } grant_write_lock(gt); if ( unlikely(op.nr_frames > gt->max_grant_frames) ) @@ -2138,6 +2174,11 @@ gnttab_query_size( } gt = d->grant_table; + if ( !gt ) + { + op.status = GNTST_bad_domain; + goto out; + } grant_read_lock(gt); @@ -2302,6 +2343,13 @@ gnttab_transfer( goto put_gfn_and_copyback; } + if ( unlikely(!e->grant_table) ) + { + gdprintk(XENLOG_INFO, "%pd has no grant table\n", e); + gop.status = GNTST_bad_domain; + goto unlock_and_copyback; + } + if ( xsm_grant_transfer(XSM_HOOK, d, e) ) { gop.status = GNTST_permission_denied; @@ -2888,6 +2936,12 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op, if ( op->flags & gref_flag ) { + if ( !buf->domain->grant_table ) + { + rc = GNTST_bad_domain; + goto out; + } + rc = acquire_grant_for_copy(buf->domain, ptr->u.ref, current->domain->domain_id, buf->read_only, @@ -3092,6 +3146,9 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop) int res; unsigned int i, nr_ents; + if ( !gt ) + return -ENODEV; + if ( copy_from_guest(&op, uop, 1) ) return -EFAULT; @@ -3270,6 +3327,11 @@ gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop, } gt = d->grant_table; + if ( !gt ) + { + op.status = GNTST_bad_domain; + goto out2; + } op.status = GNTST_okay; @@ -3332,7 +3394,11 @@ gnttab_get_version(XEN_GUEST_HANDLE_PARAM(gnttab_get_version_t) uop) return rc; } - op.version = d->grant_table->gt_version; + if ( d->grant_table ) + op.version = d->grant_table->gt_version; + else + /* Use 0 to signal no grant table. */ + op.version = 0; rcu_unlock_domain(d); @@ -3351,6 +3417,12 @@ swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b) struct active_grant_entry *act_b = NULL; s16 rc = GNTST_okay; + if ( !gt ) + { + rcu_unlock_domain(d); + return GNTST_bad_domain; + } + grant_write_lock(gt); /* Bounds check on the grant refs */ @@ -3872,6 +3944,9 @@ void grant_table_warn_active_grants(struct domain *d) #define WARN_GRANT_MAX 10 + if ( !gt ) + return; + grant_read_lock(gt); nr_ents = nr_grant_entries(gt); @@ -3953,6 +4028,9 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref, int rc = 0; uint16_t flags = 0; + if ( !gt ) + return -ENODEV; + grant_read_lock(gt); if ( gt->gt_version < 1 ) @@ -4069,6 +4147,9 @@ unsigned int gnttab_resource_max_frames(const struct domain *d, unsigned int id) const struct grant_table *gt = d->grant_table; unsigned int nr = 0; + if ( !gt ) + return 0; + /* Don't need the grant lock. This limit is fixed at domain create time. */ switch ( id ) { @@ -4100,6 +4181,9 @@ int gnttab_acquire_resource( if ( !nr_frames ) return rc; + if ( !gt ) + return -ENODEV; + final_frame = frame + nr_frames - 1; /* Grow table if necessary. */ @@ -4156,6 +4240,9 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn) struct grant_table *gt = d->grant_table; bool status = false; + if ( !gt ) + return -ENODEV; + grant_write_lock(gt); if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) ) @@ -4200,6 +4287,9 @@ static void gnttab_usage_print(struct domain *rd) struct grant_table *gt = rd->grant_table; unsigned int nr_ents; + if ( !gt ) + return; + printk(" -------- active -------- -------- shared --------\n"); printk("[ref] localdom mfn pin localdom gmfn flags\n");
Allow setting max_grant_version to 0 in order to disable grant table usage by a domain. This prevents allocating the grant-table structure inside of Xen and requires guards to be added in several functions in order to prevent dereferencing the structure. Note that a domain without a grant table could still use some of the grant related hypercalls, it could for example issue a GNTTABOP_copy of a grant reference from a remote domain into a local frame. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- docs/man/xl.cfg.5.pod.in | 4 +- tools/libs/light/libxl_dom.c | 2 +- xen/common/grant_table.c | 100 +++++++++++++++++++++++++++++++++-- 3 files changed, 98 insertions(+), 8 deletions(-)