Re: [kernel-hardening] [RFC v2 PATCH 00/13] HARDENED_ATOMIC

[Colin Vidal <colin@cvidal.org>]

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.

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.

Thanks,

Colin