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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
>
> ---
>
> Cc: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>
>     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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel