* [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS
@ 2017-10-11 14:29 Julien Grall
2017-10-11 14:51 ` Sergej Proskurin
0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2017-10-11 14:29 UTC (permalink / raw)
To: xen-devel; +Cc: Sergej Proskurin, sstabellini, Julien Grall
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS
2017-10-11 14:29 [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS Julien Grall
@ 2017-10-11 14:51 ` Sergej Proskurin
2017-10-11 14:57 ` Julien Grall
0 siblings, 1 reply; 5+ messages in thread
From: Sergej Proskurin @ 2017-10-11 14:51 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
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;
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS
2017-10-11 14:51 ` Sergej Proskurin
@ 2017-10-11 14:57 ` Julien Grall
2017-10-11 18:02 ` Sergej Proskurin
0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2017-10-11 14:57 UTC (permalink / raw)
To: Sergej Proskurin, xen-devel; +Cc: sstabellini
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;
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS
2017-10-11 14:57 ` Julien Grall
@ 2017-10-11 18:02 ` Sergej Proskurin
2017-10-11 18:47 ` Stefano Stabellini
0 siblings, 1 reply; 5+ messages in thread
From: Sergej Proskurin @ 2017-10-11 18:02 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: sstabellini
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;
>>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS
2017-10-11 18:02 ` Sergej Proskurin
@ 2017-10-11 18:47 ` Stefano Stabellini
0 siblings, 0 replies; 5+ messages in thread
From: Stefano Stabellini @ 2017-10-11 18:47 UTC (permalink / raw)
To: Sergej Proskurin; +Cc: Julien Grall, sstabellini, xen-devel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 4027 bytes --]
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;
> >>
> >
>
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-11 18:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-11 14:29 [PATCHv2 for-4.10] xen/arm: guest_walk: Fix check again the IPS Julien Grall
2017-10-11 14:51 ` Sergej Proskurin
2017-10-11 14:57 ` Julien Grall
2017-10-11 18:02 ` Sergej Proskurin
2017-10-11 18:47 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.