Re: [PATCH v3 3/3] tools: introduce parameter max_wp_ram_ranges.

["Yu, Zhang" <yu.c.zhang@linux.intel.com>]

On 2/1/2016 11:14 PM, Yu, Zhang wrote:
>
>
> On 2/1/2016 9:07 PM, Jan Beulich wrote:
>>>>> On 01.02.16 at 13:49, <wei.liu2@citrix.com> wrote:
>>> On Mon, Feb 01, 2016 at 05:15:16AM -0700, Jan Beulich wrote:
>>>>>>> On 01.02.16 at 13:02, <wei.liu2@citrix.com> wrote:
>>>>> On Mon, Feb 01, 2016 at 12:52:51AM -0700, Jan Beulich wrote:
>>>>>>>>> On 30.01.16 at 15:38, <yu.c.zhang@linux.intel.com> wrote:
>>>>>>
>>>>>>> On 1/30/2016 12:33 AM, Jan Beulich wrote:
>>>>>>>>>>> On 29.01.16 at 11:45, <yu.c.zhang@linux.intel.com> wrote:
>>>>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>>>>> @@ -940,6 +940,8 @@ static int
>>>>>>>>> hvm_ioreq_server_alloc_rangesets(struct
>>>>> hvm_ioreq_server *s,
>>>>>>>>>    {
>>>>>>>>>        unsigned int i;
>>>>>>>>>        int rc;
>>>>>>>>> +    unsigned int max_wp_ram_ranges =
>>>>>>>>> +
>>>>>>>>> s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES];
>>>>>>>>
>>>>>>>> You're still losing the upper 32 bits here. Iirc you agreed to
>>>>>>>> range
>>>>>>>> check the value before storing into params[]...
>>>>>>>
>>>>>>> Thanks, Jan. :)
>>>>>>> In this version, the check is added in routine parse_config_data().
>>>>>>> If option 'max_wp_ram_ranges' is configured with an unreasonable
>>>>>>> value,
>>>>>>> the xl will terminate, before calling xc_hvm_param_set(). Does this
>>>>>>> change meet your requirement? Or maybe did I have some
>>>>>>> misunderstanding
>>>>>>> on this issue?
>>>>>>
>>>>>> Checking in the tools is desirable, but the hypervisor shouldn't rely
>>>>>> on any tool side checking.
>>>>>>
>>>>>
>>>>> As in hypervisor needs to sanitise all input from toolstack? I don't
>>>>> think Xen does that today.
>>>>
>>>> If it doesn't, then that's a bug. Note that in many cases (domctl-s
>>>> and alike) such bogus trusting in the tool stack behaving correctly
>>>> is only not a security issue due to XSA-77. Yet with XSA-77 we
>>>> made quite clear that we shouldn't knowingly allow in further such
>>>> issues (it'll be hard enough to find and address all existing ones).
>>>
>>> So are you suggesting pulling the check done in toolstack into
>>> hypervisor?
>>
>> I think the check in the tools should stay (allowing for a
>> distinguishable error message to be issued); all I'm saying is
>> that doing the check in the tools is not enough.
>>
>> Jan
>>
>
> Thank you Jan and Wei. And sorry for the late response.
> But I still do not quite understand. :)
> If tool stack can guarantee the validity of a parameter,
> under which circumstances will hypervisor be threatened?
> I'm not familiar with XSA-77, and I'll read it ASAP.
>
> B.R.
> Yu

Sorry to bother you, Jan.
After a second thought, I guess one of the security concern
is when some APP is trying to trigger the HVMOP_set_param
directly with some illegal values.
So, we need also validate this param in hvm_allow_set_param,
current although hvm_allow_set_param has not performed any
validation other parameters. We need to do this for the new
ones. Is this understanding correct?
Another question is: as to the tool stack side, do you think
an error message would suffice? Shouldn't xl be terminated?

Thanks
Yu

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