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

[Kees Cook <keescook@chromium.org>]

On Wed, Oct 26, 2016 at 12:47 PM, Colin Vidal <colin@cvidal.org> wrote:
>> BTW, I just looked to the generic implementation of atomic64. It seems
>> quite understandable: methods use spinlock to access/modify to the
>> value of an atomic64 variable. It seems possible to check the value
>> before the increment/decrements and if the resulting value is 0, but
>> the value before the operation was different of -1 or 1, is that an
>> overflow just happened (well, it is not exactly right, but this is the
>> global idea). Hence, we revert the change, release the lock, and kill
>> the process.
>>
>> If this idea is correct, it would avoid specific implementation of
>> protected version of atomic64 for architecture with
>> GENERIC_ATOMIC64. And case (3) would be easily protected. What do you
>> think?
>
> What I am saying here is quite confusing. Here is a cleaner
> explanation:
>
>  * the generic atomic64 method enter and takes the lock
>  * before making the operation, check v->counter > INT_MAX - value (ifadd) or check v->counter < INT_MIN - value (if sub)
>  * if the previous check is true, release the lock and kill the process
>  * otherwise, let the operation process.
>
> Obviously, if this approach is not wrong, there will be a significant
> overhead, but it happens only on CONFIG_GENERIC_ATOMIC64 &&
> CONFIG_HARDENED_ATOMIC.

I think this would be fine -- though I think it should be a distinct
patch. Anything we can do to separate changes into logical chunks
makes reviewing easier.

i.e. patch ordering could look like this:

- original series with HARDENED_ATOMIC depending on !GENERIC_ATOMIC64
- implementation of protection on GENERIC_ATOMIC64, removing above
depends limitation
- ARM hardened atomic implementation

-Kees

-- 
Kees Cook
Nexus Security