Re: [Xen-devel] [PATCH 3/5] xen: few more xen_ulong_t substitutions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.12 at 17:01, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
wrote:
> On Wed, 8 Aug 2012, Jan Beulich wrote:
>> >>> On 08.08.12 at 14:12, Stefano Stabellini 
>> >>> <stefano.stabellini@xxxxxxxxxxxxx>
>> wrote:
>> > On Tue, 7 Aug 2012, Jan Beulich wrote:
>> >> > Considering that each field of a multicall_entry is usually passed as an
>> >> > hypercall parameter, they should all remain unsigned long.
>> >> 
>> >> That'll give you subtle bugs I'm afraid: do_memory_op()'s
>> >> encoding of a continuation start extent (into the 'cmd' value),
>> >> for example, depends on being able to store the full value into
>> >> the command field of the multicall structure. The limit checking
>> >> of the permitted number of extents therefore is different
>> >> between native (ULONG_MAX >> MEMOP_EXTENT_SHIFT) and
>> >> compat (UINT_MAX >> MEMOP_EXTENT_SHIFT). I would
>> >> neither find it very appealing to have do_memory_op() adjusted
>> >> for dealing with this new special case, nor am I sure that's the
>> >> only place your approach would cause problems if you excluded
>> >> the multicall structure from the model change.
>> > 
>> > Given the way the continuation is implemented, the same problem can
>> > also happen on x86.
>> 
>> No. The compat wrapper, as pointed out there has a different
>> check on the maximum number of extents, and hence the
>> continuation index can't overflow.
> 
> Right. I meant the same conceptual problem: the guest passing a number of
> extents that is too big for the hypervisor to handle.

I don't think the hypervisor has a problem handling such large
amounts.

>> > In fact, considering that we don't use any compat code, and that
>> > do_memory_op has the following check:
>> > 
>> >     /* Is size too large for us to encode a continuation? */
>> >     if ( reservation.nr_extents > (ULONG_MAX >> MEMOP_EXTENT_SHIFT) )
>> >         return start_extent;
>> > 
>> > it would work as-is for ARM too.
>> 
>> Not afaict. For a 32-bit guest, but the above code executed in
>> a 64-bit hypervisor, the guest could pass in (theoretically)
>> UINT_MAX, which would pass this check, yet the eventual
>> continuation index would get truncated when stored in the
>> 32-bit hypercall operation field.
>  
> Actually, like Ian wrote, I expect that using the upper 32 bit part of
> the x0 register, only for continuations, would work just fine.
> 
> In any case we can make that check arch dependent and restrict the
> maximum number of extents on aarch64 to the same limit we have on
> aarch32.

We should even discuss lowering the limit universally to the
UINT_MAX based value. I don't think anyone is actively using
such insane numbers.

And don't forget that I pointed out this issue only as example
of possible problems with your intended approach to the
handling of multicalls.

Jan


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