[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 26 Oct 2016 12:52:00 -0700
From: Kees Cook <keescook@...omium.org>
To: Colin Vidal <colin@...dal.org>
Cc: "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
"Reshetova, Elena" <elena.reshetova@...el.com>, David Windsor <dave@...gbits.org>
Subject: Re: [RFC v2 PATCH 00/13] HARDENED_ATOMIC
On Wed, Oct 26, 2016 at 12:47 PM, Colin Vidal <colin@...dal.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
