Re: [Xen-devel] [PATCH v2 2/4] xen: introduce grant_map_exists

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 6 Oct 2014, Jan Beulich wrote:
> >>> On 06.10.14 at 15:08, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >>> On 06.10.14 at 11:37, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> >> > On Mon, 6 Oct 2014, Jan Beulich wrote:
> >> >> >>> On 03.10.14 at 16:50, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> >> >> > +    grant_ref_t ref;
> >> >> > +    bool_t ret = 0;
> >> >> > +
> >> >> > +    ASSERT(&rgt->lock);
> >> >> > +
> >> >> > +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
> >> >> 
> >> >> This loop's worst case iteration count is controlled solely by the
> >> >> "gnttab_max_nr_frames=" command line option afaict, i.e. for a
> >> >> large enough specified value this is going to become a security
> >> >> issue.
> >> > 
> >> > I am not sure what I could do about this.
> >> > Any suggestions?
> >> 
> >> Sadly nothing simple (i.e. other than adding ways to do the reverse
> >> lookup). But I hope you agree that the lack of a good solution can't
> >> be a reason to introduce a security issue.
> > 
> > Of course.
> > 
> > 
> >> And I'm not really fancying workarounds (like limiting the maximum)
> >> here...
> > 
> > The other approach to do this, that I avoided because it is slower (2
> > spinlocks instead of 1), is to use the existing mapcount function.
> > mapcount is based on:
> > 
> > for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
> > 
> > do you think it is better?
> 
> No, that's worse: This is the Dom0 side of things, and the loop
> bound would still be controlled by that same command line option.
> 
> So between the two, the original solution would be better, and
> maybe enforcing a lower limit on DomU-s might actually be an
> option.

What about keeping track of the highest ref number actually used so far?
I could use that as upper limit instead of the theoretical max. It would
also make the execution faster.


> Btw., one more thing I think I forgot in the reply to patch 3:
> Since you intentionally only care about remote pages, shouldn't
> you check owner != d after page_get_owner_and_reference()?

The problem is that with complex scenarios, such as guest disks over
iscsi, or disk frontend and backend both in dom0, I am afraid that the
"foreign" page could actually end up belonging to the caller domain.
Also it is not really a security issue calling an hypercall to do cache
flush on your own pages, just inefficient. These are the reasons why I
removed that check.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel