[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1477512469.2263.141.camel@cvidal.org>
Date: Wed, 26 Oct 2016 22:07:49 +0200
From: Colin Vidal <colin@...dal.org>
To: Kees Cook <keescook@...omium.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, 2016-10-26 at 12:52 -0700, Kees Cook wrote:
> 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
Great!
Elena, I will wait that you applies HARDENED_ATOMIC depending on
!GENERIC_ATOMIC64, and I submit a new RFC with the implementation of
protection on GENERIC_ATOMIC64 and a v2 of ARM port. Sounds good for
everybody?
Thanks,
Colin
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.
