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.gr...@linaro.org>
---
Cc: Sergej Proskurin <prosku...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
