Re: [Xen-devel] [PATCH] Xen: ARM: Zero reserved fields of xatp before making hypervisor call

[Juergen Gross <jgross@xxxxxxxx>]

On 20/12/16 06:02, Jiandi An wrote:
> On 12/19/16 12:49, Stefano Stabellini wrote:
>> On Mon, 19 Dec 2016, Juergen Gross wrote:
>>> On 19/12/16 03:56, Jiandi An wrote:
>>>> Ensure all reserved fields of xatp are zero before making hypervisor
>>>> call to XEN in xen_map_device_mmio().  xenmem_add_to_physmap_one() in
>>>> XEN fails the mapping request if extra.res reserved field in xatp is
>>>> not zero for XENMAPSPACE_dev_mmio request.
>>>>
>>>> Signed-off-by: Jiandi An <anjiandi@xxxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/xen/arm-device.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/xen/arm-device.c b/drivers/xen/arm-device.c
>>>> index 778acf8..208273b 100644
>>>> --- a/drivers/xen/arm-device.c
>>>> +++ b/drivers/xen/arm-device.c
>>>> @@ -87,6 +87,9 @@ static int xen_map_device_mmio(const struct resource 
>>>> *resources,
>>>>                    idxs[j] = XEN_PFN_DOWN(r->start) + j;
>>>>            }
>>>>  
>>>> +          /* Ensure reserved fields are set to zero */
>>>> +          memset(&xatp, 0, sizeof(xatp));
>>>> +
>>>>            xatp.domid = DOMID_SELF;
>>>>            xatp.size = nr;
>>>>            xatp.space = XENMAPSPACE_dev_mmio;
>>>
>>> Instead of setting xatp to 0 in each iteration I'd prefer a static
>>> initialization of .domid and .space:
>>>
>>>     struct xen_add_to_physmap_range xatp = {
>>>             .domid = DOMID_SELF,
>>>             .space = XENMAPSPACE_dev_mmio
>>>     };
>>
>> +1
>>
> 
> Hi Juergen,
> 
> Thanks for you comments.  xatp is passed to XEN via the hypervisor call in 
> each loop.
> XEN touches xatp and returns it back.  For example XEN returns error of 
> underlying mapping call in the err[] array in xatp. (The err[] is not checked 
> after the hypervisor call returns and it's a bug to be addressed in a 
> separate patch)  XEN could theoretically corrupt xatp when it's returned.  
> And the loop would go on to the next iteration passing in whatever that's in 
> xatp returned by the previous hypervisor call.  Harder to debug in my opinion 
> if xatp get corrupted by XEN somehow when a bug is introduced in XEN.  At 
> first I put the memset of xatp at the beginning outside of the loop.  But I 
> thought it's better to initialize xatp that's passed in each time a 
> hypervisor call is made so we know exactly we set going into the hypervisor 
> call.
> 

It is very clearly specified which parameters are IN and which are OUT.
No trusting the hypervisor to follow this interface specification is
weird. Where do you want to stop?


Juergen

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