| Message ID | 20240311-arm32-cfi-v3-5-224a0f0a45c2@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CFI for ARM32 using LLVM | expand |
On Mon, 11 Mar 2024 at 10:15, Linus Walleij <linus.walleij@linaro.org> wrote: > > The members of the vector table arm_delay_ops are called > directly using defines, but this is really confusing for > KCFI. Wrap the calls in static inlines and tag them with > __nocfi so things start to work. > > Without this patch, platforms without a delay timer will > not boot (sticks in calibrating loop etc). > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > arch/arm/include/asm/delay.h | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > So what goes wrong without this patch? Is it really that easy to confuse kCFI? As far as I can tell (and I tried reverting it too), this patch should not be needed - in general, when all function pointer variables and the functions themselves are defined in the C domain, the compiler should be able to figure it out. Also, these are functions that are used on every system and called often and in a predictable manner, so they are prime targets for the kind of attacks that CFI is intended to harden against, so wrapping them in __nocfi is probably something we should try to avoid. > diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h > index 1d069e558d8d..7d611b810b6c 100644 > --- a/arch/arm/include/asm/delay.h > +++ b/arch/arm/include/asm/delay.h > @@ -55,7 +55,10 @@ extern struct arm_delay_ops { > unsigned long ticks_per_jiffy; > } arm_delay_ops; > > -#define __delay(n) arm_delay_ops.delay(n) > +static inline void __nocfi __delay(unsigned long n) > +{ > + arm_delay_ops.delay(n); > +} > > /* > * This function intentionally does not exist; if you see references to > @@ -76,8 +79,15 @@ extern void __bad_udelay(void); > * first constant multiplications gets optimized away if the delay is > * a constant) > */ > -#define __udelay(n) arm_delay_ops.udelay(n) > -#define __const_udelay(n) arm_delay_ops.const_udelay(n) > +static inline void __nocfi __udelay(unsigned long n) > +{ > + arm_delay_ops.udelay(n); > +} > + > +static inline void __nocfi __const_udelay(unsigned long n) > +{ > + arm_delay_ops.const_udelay(n); > +} > > #define udelay(n) \ > (__builtin_constant_p(n) ? \ > > -- > 2.34.1 >
diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h index 1d069e558d8d..7d611b810b6c 100644 --- a/arch/arm/include/asm/delay.h +++ b/arch/arm/include/asm/delay.h @@ -55,7 +55,10 @@ extern struct arm_delay_ops { unsigned long ticks_per_jiffy; } arm_delay_ops; -#define __delay(n) arm_delay_ops.delay(n) +static inline void __nocfi __delay(unsigned long n) +{ + arm_delay_ops.delay(n); +} /* * This function intentionally does not exist; if you see references to @@ -76,8 +79,15 @@ extern void __bad_udelay(void); * first constant multiplications gets optimized away if the delay is * a constant) */ -#define __udelay(n) arm_delay_ops.udelay(n) -#define __const_udelay(n) arm_delay_ops.const_udelay(n) +static inline void __nocfi __udelay(unsigned long n) +{ + arm_delay_ops.udelay(n); +} + +static inline void __nocfi __const_udelay(unsigned long n) +{ + arm_delay_ops.const_udelay(n); +} #define udelay(n) \ (__builtin_constant_p(n) ? \
The members of the vector table arm_delay_ops are called directly using defines, but this is really confusing for KCFI. Wrap the calls in static inlines and tag them with __nocfi so things start to work. Without this patch, platforms without a delay timer will not boot (sticks in calibrating loop etc). Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- arch/arm/include/asm/delay.h | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)