Re: [Xen-devel] [PATCH v18 01/10] x86: add generic resource (e.g. MSR) access hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.09.14 at 13:08, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 30/09/14 12:00, Andrew Cooper wrote:
>> On 30/09/14 11:49, Chao Peng wrote:
>>> +        /* Do sanity check earlier to omit the potential IPI overhead. */
>>> +        if ( check_resource_access(&ra) < ra.nr_entries )
>>> +        {
>>> +            /* Copy the return value for failed entry. */
>>> +            if ( __copy_to_guest_offset(guest_entries, ret,
>>> +                                        ra.entries + ret, 1) )
>>> +                ret = -EFAULT;
>>> +            else
>>> +                ret = 0;
>>> +
>>> +            xfree(ra.entries);
>>> +            break;
>>> +        }
>> This however is not ok.  You have possibly signalled that entry 0 has
>> passed the permission check and entry 1 has failed the check, at which
>> point you leave entry 0 uninitialised in userspace and signal failure
>> for entry 1.
>>
>> If some MSRs pass the permission check, you should go ahead and fill
>> them in to provide the partial good data to userspace.
> 
> Actually, on second thoughts this is not correct, because of the union
> of cmd and ret.
> 
> I cannot see a way of making this function correctly given the union, as
> there is no way of signalling "permission ok" between
> check_resource_access() and resource_access() without clobbering the
> command.
> 
> My suggestion is still as before - make use of rsvd field for ret and
> drop the union, but I would appreciate Jan's thoughts as he explicitly
> suggested the union.

See the other reply - I don't currently see the union to cause
any problem. We simply don't need to signal "permission okay"
for each individual entry, all we need is "permission okay up to
here".

Jan


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