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

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

[Jiandi An]

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@codeaurora.org>
---
 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;
-- 
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Juergen Gross]

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@codeaurora.org>
> ---
>  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
	};


Juergen

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

[Juergen Gross]

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@codeaurora.org>
> ---
>  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
	};


Juergen


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

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

[Stefano Stabellini]

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@codeaurora.org>
> > ---
> >  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

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

[Stefano Stabellini]

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@codeaurora.org>
> > ---
> >  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

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

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

[Jiandi An]

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@codeaurora.org>
>>> ---
>>>  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.

-- 
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Jiandi An]

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@codeaurora.org>
>>> ---
>>>  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.

-- 
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

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

[Juergen Gross]

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@codeaurora.org>
>>>> ---
>>>>  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

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

[Juergen Gross]

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@codeaurora.org>
>>>> ---
>>>>  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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

Hi Jiandi,

Please respect the netiquette and wrap line to 70-75 characters.

On 20/12/2016 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:

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

If you move struct xen_add_to_physmap_range in the loop, the compiler 
will initialize and zeroed for you the structure at each loop. I.e

for (i = 0; i < count; i++) {
	struct xen_add_to_physmap_range xapt = ....


}

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Jiandi,

Please respect the netiquette and wrap line to 70-75 characters.

On 20/12/2016 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:

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

If you move struct xen_add_to_physmap_range in the loop, the compiler 
will initialize and zeroed for you the structure at each loop. I.e

for (i = 0; i < count; i++) {
	struct xen_add_to_physmap_range xapt = ....


}

Cheers,

-- 
Julien Grall

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

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

[Jiandi An]

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@codeaurora.org>
---
 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;
-- 
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.


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