[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 27 Oct 2016 07:35:40 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: Colin Vidal <colin@...dal.org>, Kees Cook <keescook@...omium.org>
CC: "kernel-hardening@...ts.openwall.com"
<kernel-hardening@...ts.openwall.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?
Change pushed. Now it should be !GENERIC_ATOMIC64. Hopefully this for now concludes our state on atomic64* variables.
Now we are left with local_wrap_t problem still... But it doesn’t concern arm I think at all.
Best Regards,
Elena.
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.
