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