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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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