[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 19 Jan 2017 08:31:58 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: Eric Biggers <ebiggers3@...il.com>, "kernel-hardening@...ts.openwall.com"
<kernel-hardening@...ts.openwall.com>
CC: "keescook@...omium.org" <keescook@...omium.org>, "arnd@...db.de"
<arnd@...db.de>, "tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>, "Anvin, H Peter"
<h.peter.anvin@...el.com>, "peterz@...radead.org" <peterz@...radead.org>,
"will.deacon@....com" <will.deacon@....com>, "dwindsor@...il.com"
<dwindsor@...il.com>, "gregkh@...uxfoundation.org"
<gregkh@...uxfoundation.org>
Subject: RE: [RFCv2 PATCH 00/18] refcount_t API + usage
> On Wed, Jan 18, 2017 at 11:11:29AM +0200, Elena Reshetova wrote:
> > Changes since v1:
> > - kref INIT fixes are moved to a proper separate commit
> > - Peter's commits are now properly integrated into series
> > - various small fixes are incorporated based on testing
> > results and feedback
> >
> > This patch series is build on top of Peter's Zijlstra patches
> > that provide refcount_t type and API definition.
>
> Hi Elena,
Hi Erik,
To be honest this is all tricky and I will have to explain at least some of our reasoning below.
I guess some of us (me included) would prefer the PaX/grsecurity implementation for the main reason that
it was providing security by default to atomic_t type. That's why we first took PaX patches and did the porting
and etc. However, that approach was strongly objected and it seemed that we need to change the approach
if we want at least to start somewhere. I guess it is classical example of "disagree and commit" principle in action :)
We all afterall want a better kernel security, so small steps better than no steps.
As for this and earlier discussions I think some of your questions with regards to the initial 7 patches in the series should be answered by Peter.
> There seems to be a lot of focus on converting things to use refcount_t but much
> less focus on providing a refcount_t implementation that actually meets the
> performance and security goals of the feature.
Together with Hans we spent a lot of time working on conversions and looking on what code needs API-wise.
We haven't really put our attention to the insides of interface since Peter was behind that work and he simply
has more knowledge on the subsystem.
Notably, the proposed patchset
> provides no information about why the proposed implementation was chosen over
> the PaX implementation (note that I'm talking about the actual implementation of
> safe reference counts, not the atomic_t/atomic_unchecked_t division) which as
> I've already mentioned is much more efficient (less bloated and faster) while
> still meeting the security goal. I'm especially worried that people will be put
> in a position where they need to take performance concerns into account when
> deciding whether to use refcount_t or not.
We were initially thinking that these are just RFCs that we keep testing and polishing, so I
think maybe that's why Peter had a very short description on it. However, it does seem that it is already in linux-next now...
I guess maybe now it should be added to the documentation section? I agree that it is important to explain these things
Otherwise people either get it wrong or simply don't use refcount_t at all.
And the patch even still includes
> the "don't allow incrementing a zero refcount" check which AFAICS is bogus from
> a security perspective.
Again, we mentioned this before and I am especially worried about this part also from
practical point of view: it is such a big change that kernel might not be ready for and it
would keep coming with bugs after.
>
> Even if you and Peter disagree with the comments that I and also PaX Team have
> made, the patch must at least explain the design decisions made.
I personally don't disagree with some comments, but you are right we need more documentation provided.
Peter given that your patches are already in linux-next, where should documentation/reasoning land?
Including it to the subsystem patches doesn't seem to make any sense to me...
Best Regards,
Elena.
>
> I also find it strange that the actual refcount_t implementation is hidden in
> patch 7/18 in the series and has subject "kref: ...", when in fact it's actually
> the most important patch and will affect much more than kref.
>
> Thanks,
>
> Eric
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.
