| 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 | expand |
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;
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; >
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; >> >
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; > >> > > >
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;
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(-)