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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >>>
> 
> Reviewed-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>

Thanks Sergej, Julien. The patch is committed.


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

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