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

Message ID 20171011142902.5332-1-julien.grall@linaro.org
State New
Headers show
Series
  • [Xen-devel,PATCHv2,for-4.10] xen/arm: guest_walk: Fix check again the IPS
Related show

Commit Message

Julien Grall Oct. 11, 2017, 2:29 p.m.
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 <julien.grall@linaro.org>

---

Cc: Sergej Proskurin <proskurin@sec.in.tum.de>

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

Comments

Sergej Proskurin Oct. 11, 2017, 2:51 p.m. | #1
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 <julien.grall@linaro.org>
>
> ---
>
> Cc: Sergej Proskurin <proskurin@sec.in.tum.de>
>
>     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 Oct. 11, 2017, 2:57 p.m. | #2
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 <julien.grall@linaro.org>
>>
>> ---
>>
>> Cc: Sergej Proskurin <proskurin@sec.in.tum.de>
>>
>>      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 Oct. 11, 2017, 6:02 p.m. | #3
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 <julien.grall@linaro.org>
>>>

Reviewed-by: Sergej Proskurin <proskurin@sec.in.tum.de>

>>> ---
>>>
>>> Cc: Sergej Proskurin <proskurin@sec.in.tum.de>
>>>
>>>      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;
>>
>
Stefano Stabellini Oct. 11, 2017, 6:47 p.m. | #4
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 <julien.grall@linaro.org>

> >>>

> 

> Reviewed-by: Sergej Proskurin <proskurin@sec.in.tum.de>


Thanks Sergej, Julien. The patch is committed.


> >>> ---

> >>>

> >>> Cc: Sergej Proskurin <proskurin@sec.in.tum.de>

> >>>

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

> >>

> > 

>

Patch

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;