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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

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()?

Jan



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