RE: [kernel-hardening] [RFC v2 PATCH 00/13] HARDENED_ATOMIC

["Reshetova, Elena" <elena.reshetova@intel.com>]

>Hi Elena,

Hi Colin!

> > Perhaps the cleaner solution would be to define prototypes (of real 
> > functions, not macros) of atomic64_*_wrap functions in asm- 
> > generic/atomic64.h. If the arch has its own implementation of 
> > atomic64, no problems (asm-generic/atomic64.h is not >included), and 
> > if
> > CONFIG_GENERIC_ATOMIC64 is set, we just need to implements atomic64_*_wrap functions (in a arch/lib/atomic64.c, for instance).
> 
> Why not in lib/atomic64.c? Because this is what would be used in the case when CONFIG_GENERIC_ATOMIC is set. Also, when you say "implement", do you mean actually provide "protected" versions of wrap functions or just wrap functions themselves operating on wrap_t types but providing no difference from non-wrap functions?
> 

>The point is if CONFIG_GENERIC_ATOMIC64 is unset, asm- generic/atomic64.h and lib/atomic64.c (see lib/Makefile) are not included in compilation, therefore we don't care about that: the architecture has its own implementation of atomic64 (protected >and wrap version).

I would still break this case into two cases to have full picture:

(1) CONFIG_GENERIC_ATOMIC64 is unset, architecture has its own implementation of atomic64, but no protected and wrap versions (yet), CONFIG_ARCH_HARDENED_ATOMIC is unset and cannot be set.
(2) CONFIG_GENERIC_ATOMIC64 is unset, architecture has its own implementation of atomic64, with protected and wrap version, CONFIG_ARCH_HARDENED_ATOMIC can be both set and unset. 

The latter case is supported for x86 and work is going for arm and arm64 now. 
The first case still requires that *_wrap functions are defined in one way or another and since we cannot rely on all arch. to provide them, they are currently defined in linux/atomic.h and simply redirect to non-wrap functions. 

I guess we all agree with the above behavior?


>If CONFIG_GENERIC_ATOMIC64 is set, the common implementation is in lib/atomic64.c. Therefore, any functions in lib/atomic64.c should be renamed to be suffixed by _wrap (unprotected versions), and prototypes with _wrap suffix should be added in asm-generic/atomic64.h.

So, again for clarity, let's break this case into two cases also:
(3) CONFIG_GENERIC_ATOMIC64 is set, architecture does not provide any atomic64 implementations, CONFIG_ARCH_HARDENED_ATOMIC is set
(4) CONFIG_GENERIC_ATOMIC64 is set, architecture does not provide any atomic64 implementations, CONFIG_ARCH_HARDENED_ATOMIC is unset

The current implementation what we have in the hardened_atomic_on_next branch actually treats both of these cases in the same way (this is what Hans was explaining before): it does define atomic64*_wrap functions in both cases, but they point to the same normal functions. So, no protection provided in any case. Based on the current thread it seems that we might want to have protection in the case (3) after all, which is not really straightforward (maybe that's why grsecurity didn't have support for it).  
Also, what's why we first wanted to know how common this case (3) would be in practice? Should we just merge first the common cases and for example as Colin was proposing make it impossible for now to select CONFIG_HARDENED_ATOMIC for the case (3)? Actually I believe it is not even really possible to select it now in this case, or am I wrong? IMO we don't have anything providing CONFIG_ARCH_HARDENED_ATOMIC in this case. 

>The protected versions of functions depends of each architecture, therefore they can't be generic. That why I propose to add implements them in arch/lib/atomic64.c (or even arch/lib/atomic64_wrap.c, it is much clear).
>And yes, by "implement" I means write "protected" versions. Err... Yes, in that case, we don't have the "examples" implemented in PAX. But the other solution would be leave GENERIC_ATOMIC64 unprotected, which is really unclear from the "user" point->of-view.
>If CONFIG_GENERIC_ATOMIC64 is set and CONFIG_HARDENED_ATOMIC is unset, the implementations in arch/lib/atomic64_wrap.c just does not include the overflow test on the add/sub operations, like the current protected
>arm/x64 implementations.

Not sure I fully follow here... In x86 case  rch-specific atomic64* functions are defined in arch/x86/include/asm/atomic64_32/64.h. What location would correspond to arch/lib/atomic64.c in that case?

> Overall, I agree, there is no magic, but IMO definitions are so confusing even without wrap added (local_t and its functions is a good example), that tracking them all down is a pain. 
> At some point we used this table to track functions and their 
> definition:  
> https://docs.google.com/spreadsheets/d/12wX-csPY8tnHpQVRKVFPSOoV8o6WXt
> AwKKTAkx8uUIQ/edit?usp=sharing Maybe it can be extended to include 
> more info that people would like to see there.

>That would be useful and avoid numerous post-it on my desk :-) Thanks!

So, what info do you want to see there? :) Should I make one more table with different CONFIG on/off options and link where function is defined/implemented in that case?

Best Regards,
Elena.