Re: [Xen-devel] [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS

2017-10-11 Thread Stefano Stabellini 
On Wed, 11 Oct 2017, Sergej Proskurin wrote:
> Hi Julien,
> 
> On 10/11/2017 04:57 PM, Julien Grall wrote:
> > 
> > 
> > On 11/10/17 15:51, Sergej Proskurin wrote:
> >> Hi Julien,
> > 
> > Hi,
> > 
> >> On 10/11/2017 04:29 PM, Julien Grall wrote:
> >>> The function get_ipa_output_size is check whether the input size
> >>> configured by the guest is valid and will return it.
> >>>
> >>> The check is done with the IPS already shifted against
> >>> TCR_EL1_IPS_48_BIT. However the constant has been defined with the
> >>> shift included, resulting the check always been false.
> >>>
> >>> Fix it by doing the check on the non-shifted value.
> >>>
> >>> This was introduced by commit 7d623b358a "arm/mem_access: Add
> >>> long-descriptor
> >>> based gpt" introduced software page-table walk for stage-1.
> >>>
> >>> Note that the IPS code is now surrounded with #ifdef CONFIG_ARM_64
> >>> because the Arm32 compiler will complain of shift bigger than the width
> >>> of the variable. This is fine as the code is executed for 64-bit
> >>> domain only.
> >>
> >> This is a bit controversial as compared to your review comments to the
> >> initial implementation. You did not want to see any #define
> >> CONFIG_ARM_64 within the code. TCR_EL1 is a 64-bit Register: to prevent
> >> compilation issues for Aarch32 systems, why don't you use uint64_t for
> >> ips instead of register_t?
> > 
> > I am fully aware what I said in the previous reviews and I still took
> > this decision because you will mix uint64_t and register_t. #ifdef
> > CONFIG_ARM_64 is much nicer than mixing types.
> > 
> > Another way to fix it would be to rework completely the way you did
> > introduce TCR_EL1_IPS_*_BIT so you stick with non-shifted value rather
> > than shifted one.
> > 
> > But I don't have time for that and I don't want to see a latent security
> > bug in the release.
> > 
> > Cheers,
> > 
> >> Thanks,
> >> ~Sergej
> >>
> >>>
> >>> Coverity-ID: 1457707
> >>> Signed-off-by: Julien Grall 
> >>>
> 
> Reviewed-by: Sergej Proskurin 

Thanks Sergej, Julien. The patch is committed.


> >>> ---
> >>>
> >>> Cc: Sergej Proskurin 
> >>>
> >>>  Changes in v2:
> >>>  - Fix compilation on Arm32
> >>> ---
> >>>   xen/arch/arm/guest_walk.c | 8 +---
> >>>   1 file changed, 5 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> >>> index c38bedcf65..4d1ea0cdc1 100644
> >>> --- a/xen/arch/arm/guest_walk.c
> >>> +++ b/xen/arch/arm/guest_walk.c
> >>> @@ -185,7 +185,8 @@ static int guest_walk_sd(const struct vcpu *v,
> >>>   static int get_ipa_output_size(struct domain *d, register_t tcr,
> >>>  unsigned int *output_size)
> >>>   {
> >>> -    unsigned int ips;
> >>> +#ifdef CONFIG_ARM_64
> >>> +    register_t ips;
> >>>     static const unsigned int ipa_sizes[7] = {
> >>>   TCR_EL1_IPS_32_BIT_VAL,
> >>> @@ -200,7 +201,7 @@ static int get_ipa_output_size(struct domain *d,
> >>> register_t tcr,
> >>>   if ( is_64bit_domain(d) )
> >>>   {
> >>>   /* Get the intermediate physical address size. */
> >>> -    ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT;
> >>> +    ips = tcr & TCR_EL1_IPS_MASK;
> >>>     /*
> >>>    * Return an error on reserved IPA output-sizes and if the IPA
> >>> @@ -211,9 +212,10 @@ static int get_ipa_output_size(struct domain *d,
> >>> register_t tcr,
> >>>   if ( ips > TCR_EL1_IPS_48_BIT )
> >>>   return -EFAULT;
> >>>   -    *output_size = ipa_sizes[ips];
> >>> +    *output_size = ipa_sizes[ips >> TCR_EL1_IPS_SHIFT];
> >>>   }
> >>>   else
> >>> +#endif
> >>>   *output_size = TCR_EL1_IPS_40_BIT_VAL;
> >>>     return 0;
> >>
> > 
> ___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS

2017-10-11 Thread Sergej Proskurin 
Hi Julien,

On 10/11/2017 04:57 PM, Julien Grall wrote:
> 
> 
> On 11/10/17 15:51, Sergej Proskurin wrote:
>> Hi Julien,
> 
> Hi,
> 
>> On 10/11/2017 04:29 PM, Julien Grall wrote:
>>> The function get_ipa_output_size is check whether the input size
>>> configured by the guest is valid and will return it.
>>>
>>> The check is done with the IPS already shifted against
>>> TCR_EL1_IPS_48_BIT. However the constant has been defined with the
>>> shift included, resulting the check always been false.
>>>
>>> Fix it by doing the check on the non-shifted value.
>>>
>>> This was introduced by commit 7d623b358a "arm/mem_access: Add
>>> long-descriptor
>>> based gpt" introduced software page-table walk for stage-1.
>>>
>>> Note that the IPS code is now surrounded with #ifdef CONFIG_ARM_64
>>> because the Arm32 compiler will complain of shift bigger than the width
>>> of the variable. This is fine as the code is executed for 64-bit
>>> domain only.
>>
>> This is a bit controversial as compared to your review comments to the
>> initial implementation. You did not want to see any #define
>> CONFIG_ARM_64 within the code. TCR_EL1 is a 64-bit Register: to prevent
>> compilation issues for Aarch32 systems, why don't you use uint64_t for
>> ips instead of register_t?
> 
> I am fully aware what I said in the previous reviews and I still took
> this decision because you will mix uint64_t and register_t. #ifdef
> CONFIG_ARM_64 is much nicer than mixing types.
> 
> Another way to fix it would be to rework completely the way you did
> introduce TCR_EL1_IPS_*_BIT so you stick with non-shifted value rather
> than shifted one.
> 
> But I don't have time for that and I don't want to see a latent security
> bug in the release.
> 
> Cheers,
> 
>> Thanks,
>> ~Sergej
>>
>>>
>>> Coverity-ID: 1457707
>>> Signed-off-by: Julien Grall 
>>>

Reviewed-by: Sergej Proskurin 

>>> ---
>>>
>>> Cc: Sergej Proskurin 
>>>
>>>  Changes in v2:
>>>  - Fix compilation on Arm32
>>> ---
>>>   xen/arch/arm/guest_walk.c | 8 +---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
>>> index c38bedcf65..4d1ea0cdc1 100644
>>> --- a/xen/arch/arm/guest_walk.c
>>> +++ b/xen/arch/arm/guest_walk.c
>>> @@ -185,7 +185,8 @@ static int guest_walk_sd(const struct vcpu *v,
>>>   static int get_ipa_output_size(struct domain *d, register_t tcr,
>>>  unsigned int *output_size)
>>>   {
>>> -    unsigned int ips;
>>> +#ifdef CONFIG_ARM_64
>>> +    register_t ips;
>>>     static const unsigned int ipa_sizes[7] = {
>>>   TCR_EL1_IPS_32_BIT_VAL,
>>> @@ -200,7 +201,7 @@ static int get_ipa_output_size(struct domain *d,
>>> register_t tcr,
>>>   if ( is_64bit_domain(d) )
>>>   {
>>>   /* Get the intermediate physical address size. */
>>> -    ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT;
>>> +    ips = tcr & TCR_EL1_IPS_MASK;
>>>     /*
>>>    * Return an error on reserved IPA output-sizes and if the IPA
>>> @@ -211,9 +212,10 @@ static int get_ipa_output_size(struct domain *d,
>>> register_t tcr,
>>>   if ( ips > TCR_EL1_IPS_48_BIT )
>>>   return -EFAULT;
>>>   -    *output_size = ipa_sizes[ips];
>>> +    *output_size = ipa_sizes[ips >> TCR_EL1_IPS_SHIFT];
>>>   }
>>>   else
>>> +#endif
>>>   *output_size = TCR_EL1_IPS_40_BIT_VAL;
>>>     return 0;
>>
> 

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

Re: [Xen-devel] [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS

2017-10-11 Thread Julien Grall 



On 11/10/17 15:51, Sergej Proskurin wrote:

Hi Julien,


Hi,


On 10/11/2017 04:29 PM, Julien Grall wrote:

The function get_ipa_output_size is check whether the input size
configured by the guest is valid and will return it.

The check is done with the IPS already shifted against
TCR_EL1_IPS_48_BIT. However the constant has been defined with the
shift included, resulting the check always been false.

Fix it by doing the check on the non-shifted value.

This was introduced by commit 7d623b358a "arm/mem_access: Add long-descriptor
based gpt" introduced software page-table walk for stage-1.

Note that the IPS code is now surrounded with #ifdef CONFIG_ARM_64
because the Arm32 compiler will complain of shift bigger than the width
of the variable. This is fine as the code is executed for 64-bit domain only.


This is a bit controversial as compared to your review comments to the
initial implementation. You did not want to see any #define
CONFIG_ARM_64 within the code. TCR_EL1 is a 64-bit Register: to prevent
compilation issues for Aarch32 systems, why don't you use uint64_t for
ips instead of register_t?


I am fully aware what I said in the previous reviews and I still took 
this decision because you will mix uint64_t and register_t. #ifdef 
CONFIG_ARM_64 is much nicer than mixing types.


Another way to fix it would be to rework completely the way you did 
introduce TCR_EL1_IPS_*_BIT so you stick with non-shifted value rather 
than shifted one.


But I don't have time for that and I don't want to see a latent security 
bug in the release.


Cheers,


Thanks,
~Sergej



Coverity-ID: 1457707
Signed-off-by: Julien Grall 

---

Cc: Sergej Proskurin 

 Changes in v2:
 - Fix compilation on Arm32
---
  xen/arch/arm/guest_walk.c | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index c38bedcf65..4d1ea0cdc1 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -185,7 +185,8 @@ static int guest_walk_sd(const struct vcpu *v,
  static int get_ipa_output_size(struct domain *d, register_t tcr,
 unsigned int *output_size)
  {
-unsigned int ips;
+#ifdef CONFIG_ARM_64
+register_t ips;
  
  static const unsigned int ipa_sizes[7] = {

  TCR_EL1_IPS_32_BIT_VAL,
@@ -200,7 +201,7 @@ static int get_ipa_output_size(struct domain *d, register_t 
tcr,
  if ( is_64bit_domain(d) )
  {
  /* Get the intermediate physical address size. */
-ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT;
+ips = tcr & TCR_EL1_IPS_MASK;
  
  /*

   * Return an error on reserved IPA output-sizes and if the IPA
@@ -211,9 +212,10 @@ static int get_ipa_output_size(struct domain *d, 
register_t tcr,
  if ( ips > TCR_EL1_IPS_48_BIT )
  return -EFAULT;
  
-*output_size = ipa_sizes[ips];

+*output_size = ipa_sizes[ips >> TCR_EL1_IPS_SHIFT];
  }
  else
+#endif
  *output_size = TCR_EL1_IPS_40_BIT_VAL;
  
  return 0;




--
Julien Grall

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

Re: [Xen-devel] [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS

2017-10-11 Thread Sergej Proskurin 
Hi Julien,


On 10/11/2017 04:29 PM, Julien Grall wrote:
> The function get_ipa_output_size is check whether the input size
> configured by the guest is valid and will return it.
>
> The check is done with the IPS already shifted against
> TCR_EL1_IPS_48_BIT. However the constant has been defined with the
> shift included, resulting the check always been false.
>
> Fix it by doing the check on the non-shifted value.
>
> This was introduced by commit 7d623b358a "arm/mem_access: Add long-descriptor
> based gpt" introduced software page-table walk for stage-1.
>
> Note that the IPS code is now surrounded with #ifdef CONFIG_ARM_64
> because the Arm32 compiler will complain of shift bigger than the width
> of the variable. This is fine as the code is executed for 64-bit domain only.

This is a bit controversial as compared to your review comments to the
initial implementation. You did not want to see any #define
CONFIG_ARM_64 within the code. TCR_EL1 is a 64-bit Register: to prevent
compilation issues for Aarch32 systems, why don't you use uint64_t for
ips instead of register_t?

Thanks,
~Sergej

>
> Coverity-ID: 1457707
> Signed-off-by: Julien Grall 
>
> ---
>
> Cc: Sergej Proskurin 
>
> Changes in v2:
> - Fix compilation on Arm32
> ---
>  xen/arch/arm/guest_walk.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index c38bedcf65..4d1ea0cdc1 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -185,7 +185,8 @@ static int guest_walk_sd(const struct vcpu *v,
>  static int get_ipa_output_size(struct domain *d, register_t tcr,
> unsigned int *output_size)
>  {
> -unsigned int ips;
> +#ifdef CONFIG_ARM_64
> +register_t ips;
>  
>  static const unsigned int ipa_sizes[7] = {
>  TCR_EL1_IPS_32_BIT_VAL,
> @@ -200,7 +201,7 @@ static int get_ipa_output_size(struct domain *d, 
> register_t tcr,
>  if ( is_64bit_domain(d) )
>  {
>  /* Get the intermediate physical address size. */
> -ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT;
> +ips = tcr & TCR_EL1_IPS_MASK;
>  
>  /*
>   * Return an error on reserved IPA output-sizes and if the IPA
> @@ -211,9 +212,10 @@ static int get_ipa_output_size(struct domain *d, 
> register_t tcr,
>  if ( ips > TCR_EL1_IPS_48_BIT )
>  return -EFAULT;
>  
> -*output_size = ipa_sizes[ips];
> +*output_size = ipa_sizes[ips >> TCR_EL1_IPS_SHIFT];
>  }
>  else
> +#endif
>  *output_size = TCR_EL1_IPS_40_BIT_VAL;
>  
>  return 0;

-- 
Sergej Proskurin, M.Sc.
Wissenschaftlicher Mitarbeiter

Technische Universität München
Fakultät für Informatik
Lehrstuhl für Sicherheit in der Informatik

Boltzmannstraße 3
85748 Garching (bei München)

Tel. +49 (0)89 289-18592
Fax +49 (0)89 289-18579



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

[Xen-devel] [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS

2017-10-11 Thread Julien Grall 
The function get_ipa_output_size is check whether the input size
configured by the guest is valid and will return it.

The check is done with the IPS already shifted against
TCR_EL1_IPS_48_BIT. However the constant has been defined with the
shift included, resulting the check always been false.

Fix it by doing the check on the non-shifted value.

This was introduced by commit 7d623b358a "arm/mem_access: Add long-descriptor
based gpt" introduced software page-table walk for stage-1.

Note that the IPS code is now surrounded with #ifdef CONFIG_ARM_64
because the Arm32 compiler will complain of shift bigger than the width
of the variable. This is fine as the code is executed for 64-bit domain only.

Coverity-ID: 1457707
Signed-off-by: Julien Grall 

---

Cc: Sergej Proskurin 

Changes in v2:
- Fix compilation on Arm32
---
 xen/arch/arm/guest_walk.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index c38bedcf65..4d1ea0cdc1 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -185,7 +185,8 @@ static int guest_walk_sd(const struct vcpu *v,
 static int get_ipa_output_size(struct domain *d, register_t tcr,
unsigned int *output_size)
 {
-unsigned int ips;
+#ifdef CONFIG_ARM_64
+register_t ips;
 
 static const unsigned int ipa_sizes[7] = {
 TCR_EL1_IPS_32_BIT_VAL,
@@ -200,7 +201,7 @@ static int get_ipa_output_size(struct domain *d, register_t 
tcr,
 if ( is_64bit_domain(d) )
 {
 /* Get the intermediate physical address size. */
-ips = (tcr & TCR_EL1_IPS_MASK) >> TCR_EL1_IPS_SHIFT;
+ips = tcr & TCR_EL1_IPS_MASK;
 
 /*
  * Return an error on reserved IPA output-sizes and if the IPA
@@ -211,9 +212,10 @@ static int get_ipa_output_size(struct domain *d, 
register_t tcr,
 if ( ips > TCR_EL1_IPS_48_BIT )
 return -EFAULT;
 
-*output_size = ipa_sizes[ips];
+*output_size = ipa_sizes[ips >> TCR_EL1_IPS_SHIFT];
 }
 else
+#endif
 *output_size = TCR_EL1_IPS_40_BIT_VAL;
 
 return 0;
-- 
2.11.0


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