[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2236FBA76BA1254E88B949DDB74E612B41BF8C68@IRSMSX102.ger.corp.intel.com>
Date: Wed, 26 Oct 2016 08:17:44 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: Colin Vidal <colin@...dal.org>, "kernel-hardening@...ts.openwall.com"
<kernel-hardening@...ts.openwall.com>
CC: Kees Cook <keescook@...omium.org>, David Windsor <dave@...gbits.org>
Subject: RE: [RFC v2 PATCH 00/13] HARDENED_ATOMIC
On Tue, 2016-10-25 at 13:51 -0400, David Windsor wrote:
> On Tue, Oct 25, 2016 at 1:18 PM, Colin Vidal <colin@...dal.org> wrote:
> >
> > Hi Kees, Hans,
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > This series brings the PaX/Grsecurity PAX_REFCOUNT feature
> > > > > > support to the upstream kernel. All credit for the feature
> > > > > > goes to the feature authors.
> > > > > >
> > > > > > The name of the upstream feature is HARDENED_ATOMIC and it
> > > > > > is configured using CONFIG_HARDENED_ATOMIC and
> > > > > > HAVE_ARCH_HARDENED_ATOMIC.
> > > > > >
> > > > > > This series only adds x86 support; other architectures are
> > > > > > expected to add similar support gradually.
> > > > >
> > > > > I have some worries on the generic arch independent
> > > > > implementation of atomic64_t/atomic64_wrap_t
> > > > > (include/asm-generic/atomic64.h). We provide _wrap versions
> > > > > for atomic64, but protection is dependant on arch
> > > > > implementation and config. That is, one could possibly
> > > > > implement HARDENED_ATOMIC support while leaving atomic64_t
> > > > > unprotected depending on specific configs, for instance by then defaulting to CONFIG_GENERIC_ATOMIC64 (in linuc/hardened/atomic.h:676). Or maybe I'm just under-/overthinking this?
> > > >
> > > > IIUC, ARMv6 builds could have GENERIC_ATOMIC64 and (once
> > > > implemented) HARDENED_ATOMIC, so I think that combination is
> > > > worth spending time on.
> > >
> > > I'm not completely sure what you mean? Our current patchset
> > > doesn't implement any protections for the generic atomic64, but
> > > rather relies on HARDENED_ATOMIC enabled archs to provide a
> > > protected implementation. So currently any HARDENED_ATOMIC archs
> > > cannot depend on GENERIC_ATOMIC64. Does this sound reasonable?
> >
> > In the actual situation, you can use a architecture with
> > GENERIC_ATOMIC64 (imx_v6_v7_defconfig on arm for instance), and set
> > CONFIG_HARDENED_ATOMIC=y. That will broke the build. Therefore, we
> > should put a negative dependency between GENERIC_ATOMIC64 and
> > HAVE_ARCH_HARDENED_ATOMIC, in order to be sure that HARDENED_ATOMIC
> > cannot be set when GENERIC_ATOMIC64 is set.
> >
>
> This is starting to get out of hand. I'm reviewing the situation now
> as it relates to local_wrap_t getting defined in certain
> circumstances, but not others, and have found that the dependency
> resolution scheme in HARDENED_ATOMIC is confusing. I think we need to
> document this somewhere. If not in-tree, then on this mailing list,
> at least.
>
> If you have a solid understanding of what types get defined when
> architectural support for CONFIG_HARDENED_ATOMIC is enabled, where
> those types get defined (arch-specific vs. arch-independent code),
> would you mind writing something up here for all of us? Also, it very
> well could be the case that this is easier than I'm making it out to
> be. If so, just tell me and I'll go away.
>If you have CONFIG_HARDENED_ATOMIC, the type of atomic64_wrap_t is struct atomic64_wrap_t, otherwise, it is just an alias (compatible) of atomic64_t.
>So, if CONFIG_GENERIC_ATOMIC64 is set, and CONFIG_HARDENED_ATOMIC is set too, all macro definitions atomic_*_wrap in asm-generic/atomic64.h will be reduced into atomic_*. But as CONFIG_HARDENED_ATOMIC is set, the type of argument is >atomic64_wrap_t (which is different and incompatible with atomic64_t in that case). So the compile error is obvious.
>I think (someone else too, Mark if I remember?) that we should never use "typedef atomic_t atomic_wrap_t" and so on for two main reasons :
>- it avoid to mix atomic_t and atomic_wrap_t without error if CONFIG_HARDENED_ATOMIC is unset
>- it make the typing system of atomic_* much more clear: atomic*_wrap_t is always a different type of atomic_t, and does not depends of a compile option. Therefore, it would avoid dead-end like asm- generic/atomic64.h
> >
> > But it seems wired, or a pity, that HARDENED_ATOMIC is disabled on
> > some architecture just because code implementation issues, no?
> >
> > >
> > > >
> > > > >
> > > > > My concern is that this is a very easy place to include errors
> > > > > and inconsistencies. We've been trying to cleanly fix this,
> > > > > but haven't really found a satisfactory solution (e.g. one
> > > > > that actually works on different configs/arcs and isn't a
> > > > > horrible mess). I recall that the hardened_atomic ARM
> > > > > implementation already faced issues with atomic64, so this seems to be a real cause for problems. Any suggestions on how to do this more cleanly?
> > > >
> > > > I haven't looked too closely yet, though maybe Colin will have
> > > > some thoughts as he looks at the ARM port.
> > >
> > > Ok, that would probably be helpful. It would be good to get this
> > > cleanly done from the start so it doesn't grow increasingly messy
> > > with every arch needing to do incremental fixes/hacks as they get implemented.
> >
> > Since GENERIC_ATOMIC64 is only on few architecture (arm, metatag,
> > microblaze, sparc, and perhaps mips?), I wonder if it would not be a
> > better idea to drop asm-generic/atomic64.h: it will induces a code
> > duplication, for sure, but avoid the wired situation above.
> >
> > That said, I don't really understand how asm-generic/atomic64.h works:
> > it defines lot of extern functions (atomic64_add, for instance) and
> > a can't find the implementation in the arch directory (in sparc, for
> > instance)... Some ideas? It could be an interesting workaround:
> > define atomic64_*_wrap prototypes in asm-generic/atomic64.h, and
> > each architecture with GENERIC_ATOMIC64 must implement them.
> >
>
> I don't think anyone else understands, either. It would be great to
> have some documentation of how this all works.
>I just found it: lib/atomic64.c. Phew, no magics :-)
>Perhaps the cleaner solution would be to define prototypes (of real functions, not macros) of atomic64_*_wrap functions in asm- generic/atomic64.h. If the arch has its own implementation of atomic64, no problems (asm-generic/atomic64.h is not >included), and if
>CONFIG_GENERIC_ATOMIC64 is set, we just need to implements atomic64_*_wrap functions (in a arch/lib/atomic64.c, for instance).
Why not in lib/atomic64.c? Because this is what would be used in the case when CONFIG_GENERIC_ATOMIC is set. Also, when you say "implement", do you mean actually provide "protected" versions of wrap functions or just wrap functions themselves operating on wrap_t types but providing no difference from non-wrap functions?
Overall, I agree, there is no magic, but IMO definitions are so confusing even without wrap added (local_t and its functions is a good example), that tracking them all down is a pain.
At some point we used this table to track functions and their definition: https://docs.google.com/spreadsheets/d/12wX-csPY8tnHpQVRKVFPSOoV8o6WXtAwKKTAkx8uUIQ/edit?usp=sharing
Maybe it can be extended to include more info that people would like to see there.
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.
