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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.


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

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