XENMEM_maximum_gpfn return value

XENMEM_maximum_gpfn return value

[Jan Beulich]

This coming from a shared info field, it has to be assumed to possibly
have any value. In particular, our kernels don't set up this field at all
if running as Dom0 and kexec isn't enabled (along with not setting up
the P2M frame lists, as that's simply wasted memory in that case).

So, with this being a guest provided value, we apparently have two
problems: do_memory_op()'s "rc" variable is only of type "int", thus
potentially truncating the result of domain_get_maximum_gpfn()
(considering that we now support 16Tb, an 8Tb+ guest ought to
be possible, and such would have a max GPFN with 32 significant
bits). And zero or a very large number being returned by the latter
will generally be mistaken as an error code by the caller of the
hypercall.

So, along with promoting "rc" to long, I'm considering enforcing a
sane lower bound on domain_get_maximum_gpfn()'s return value,
using d->tot_pages (under the assumption that each page would
have a representation in the physical map, and hence the
physical map can't reasonably be smaller than this). That would
overcome the subtraction of 1 done there for PV guests to
convert 0 to -1 (i.e. -EPERM). Similarly, a sane upper bound
ought to be enforced (to avoid collisions with the -E range).

Other thoughts? Is such a behavioral change acceptable for an
existing interface?

Jan

Re: XENMEM_maximum_gpfn return value

[Tim Deegan]

At 14:33 +0000 on 25 Feb (1361802812), Jan Beulich wrote:
> This coming from a shared info field, it has to be assumed to possibly
> have any value. In particular, our kernels don't set up this field at all
> if running as Dom0 and kexec isn't enabled (along with not setting up
> the P2M frame lists, as that's simply wasted memory in that case).
> 
> So, with this being a guest provided value, we apparently have two
> problems: do_memory_op()'s "rc" variable is only of type "int", thus
> potentially truncating the result of domain_get_maximum_gpfn()
> (considering that we now support 16Tb, an 8Tb+ guest ought to
> be possible, and such would have a max GPFN with 32 significant
> bits). And zero or a very large number being returned by the latter
> will generally be mistaken as an error code by the caller of the
> hypercall.
> 
> So, along with promoting "rc" to long, I'm considering enforcing a
> sane lower bound on domain_get_maximum_gpfn()'s return value,
> using d->tot_pages (under the assumption that each page would
> have a representation in the physical map, and hence the
> physical map can't reasonably be smaller than this). That would
> overcome the subtraction of 1 done there for PV guests to
> convert 0 to -1 (i.e. -EPERM). Similarly, a sane upper bound
> ought to be enforced (to avoid collisions with the -E range).
> 
> Other thoughts? Is such a behavioral change acceptable for an
> existing interface?

The new behaviour seems sensible but I'm a little worried about changing
the behaviour of an existing call.  I'd be inclined to add a new
hypercall that DTRT and leave the old one, deprecated.

Tim.

Re: XENMEM_maximum_gpfn return value

[Jan Beulich]

>>> On 26.02.13 at 16:27, Tim Deegan <tim@xen.org> wrote:
> At 14:33 +0000 on 25 Feb (1361802812), Jan Beulich wrote:
>> This coming from a shared info field, it has to be assumed to possibly
>> have any value. In particular, our kernels don't set up this field at all
>> if running as Dom0 and kexec isn't enabled (along with not setting up
>> the P2M frame lists, as that's simply wasted memory in that case).
>> 
>> So, with this being a guest provided value, we apparently have two
>> problems: do_memory_op()'s "rc" variable is only of type "int", thus
>> potentially truncating the result of domain_get_maximum_gpfn()
>> (considering that we now support 16Tb, an 8Tb+ guest ought to
>> be possible, and such would have a max GPFN with 32 significant
>> bits). And zero or a very large number being returned by the latter
>> will generally be mistaken as an error code by the caller of the
>> hypercall.
>> 
>> So, along with promoting "rc" to long, I'm considering enforcing a
>> sane lower bound on domain_get_maximum_gpfn()'s return value,
>> using d->tot_pages (under the assumption that each page would
>> have a representation in the physical map, and hence the
>> physical map can't reasonably be smaller than this). That would
>> overcome the subtraction of 1 done there for PV guests to
>> convert 0 to -1 (i.e. -EPERM). Similarly, a sane upper bound
>> ought to be enforced (to avoid collisions with the -E range).
>> 
>> Other thoughts? Is such a behavioral change acceptable for an
>> existing interface?
> 
> The new behaviour seems sensible but I'm a little worried about changing
> the behaviour of an existing call.  I'd be inclined to add a new
> hypercall that DTRT and leave the old one, deprecated.

So I think this should be two steps then - fix the current call (so
that it doesn't return -1 (== -EPERM) anymore when the field is
zero, and so it doesn't truncate big values anymore) and, should
it ever turn out necessary, add a new one returning a sanitized
value.

I'll send a patch for the first part in a minute, but I'll leave the
second step open as I think the context in which the problem
was observed (xen-mceinj) is actually in need of fixing itself (i.e.
should not be using this call).

Jan