Value of macro MIDR_IMPLEMENTOR_MASK exceeds the range of integer
and can lead to overflow. Currently there is no issue as it is used
in an expression implicitly casted to u32 in MIDR_IS_CPU_MODEL_RANGE.
To avoid possible problems, fix the macro.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/include/asm/processor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index 8ab2940f68..149fae0d27 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -39,7 +39,7 @@
 #define MIDR_VARIANT(midr) \
     (((midr) & MIDR_VARIANT_MASK) >> MIDR_VARIANT_SHIFT)
 #define MIDR_IMPLEMENTOR_SHIFT  24
-#define MIDR_IMPLEMENTOR_MASK   (0xff << MIDR_IMPLEMENTOR_SHIFT)
+#define MIDR_IMPLEMENTOR_MASK   (0xffu << MIDR_IMPLEMENTOR_SHIFT)
 #define MIDR_IMPLEMENTOR(midr) \
     (((midr) & MIDR_IMPLEMENTOR_MASK) >> MIDR_IMPLEMENTOR_SHIFT)
 
-- 
2.29.0

Hi Michal,

On 25/02/2022 08:38, Michal Orzel wrote:
> Value of macro MIDR_IMPLEMENTOR_MASK exceeds the range of integer
> and can lead to overflow. Currently there is no issue as it is used
> in an expression implicitly casted to u32 in MIDR_IS_CPU_MODEL_RANGE.
> To avoid possible problems, fix the macro.

Correct me if I am wrong, it sounds like this is only for hardening 
purpose at the moment.

As this code is coming from Linux, I would prefer if we first upstream 
to Linux and then port to Xen once merged.

> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
>   xen/arch/arm/include/asm/processor.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
> index 8ab2940f68..149fae0d27 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -39,7 +39,7 @@
>   #define MIDR_VARIANT(midr) \
>       (((midr) & MIDR_VARIANT_MASK) >> MIDR_VARIANT_SHIFT)
>   #define MIDR_IMPLEMENTOR_SHIFT  24
> -#define MIDR_IMPLEMENTOR_MASK   (0xff << MIDR_IMPLEMENTOR_SHIFT)
> +#define MIDR_IMPLEMENTOR_MASK   (0xffu << MIDR_IMPLEMENTOR_SHIFT)

NIT: We tend to use 0xffU.

>   #define MIDR_IMPLEMENTOR(midr) \
>       (((midr) & MIDR_IMPLEMENTOR_MASK) >> MIDR_IMPLEMENTOR_SHIFT)
>   

Cheers,

-- 
Julien Grall

On 25/02/2022 10:54, Julien Grall wrote:
> Hi Michal,
>
> On 25/02/2022 08:38, Michal Orzel wrote:
>> Value of macro MIDR_IMPLEMENTOR_MASK exceeds the range of integer
>> and can lead to overflow. Currently there is no issue as it is used
>> in an expression implicitly casted to u32 in MIDR_IS_CPU_MODEL_RANGE.
>> To avoid possible problems, fix the macro.
>
> Correct me if I am wrong, it sounds like this is only for hardening
> purpose at the moment.
>
> As this code is coming from Linux, I would prefer if we first upstream
> to Linux and then port to Xen once merged.

Well.  The expression is undefined behaviour in C, because of shifting
into the sign bit.

In principle, the compiler is free to optimise is_affected_midr_range()
to "return true" as a consequence, even if compilers tend not to be that
malicious.

~Andrew

Hi,

On 25/02/2022 10:59, Andrew Cooper wrote:
> On 25/02/2022 10:54, Julien Grall wrote:
>> Hi Michal,
>>
>> On 25/02/2022 08:38, Michal Orzel wrote:
>>> Value of macro MIDR_IMPLEMENTOR_MASK exceeds the range of integer
>>> and can lead to overflow. Currently there is no issue as it is used
>>> in an expression implicitly casted to u32 in MIDR_IS_CPU_MODEL_RANGE.
>>> To avoid possible problems, fix the macro.
>>
>> Correct me if I am wrong, it sounds like this is only for hardening
>> purpose at the moment.
>>
>> As this code is coming from Linux, I would prefer if we first upstream
>> to Linux and then port to Xen once merged.
> 
> Well.  The expression is undefined behaviour in C, because of shifting
> into the sign bit.
> 
> In principle, the compiler is free to optimise is_affected_midr_range()
> to "return true" as a consequence, even if compilers tend not to be that
> malicious.

Are you arguing against fixing Linux first and the backport it to Xen?

Cheers,

-- 
Julien Grall