Re: [Xen-devel] [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS
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
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
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
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
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