| Message ID | 20220225083854.6371-1-michal.orzel@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Avoid integer overflow using MIDR_IMPLEMENTOR_MASK | expand |
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,
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,
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)
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(-)