Re: [PATCH 1/4] locking/atomic/x86: Silence intentional wrapping addition

[Kees Cook <keescook@chromium.org>]

On Thu, Apr 25, 2024 at 12:54:36AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 24, 2024 at 03:45:07PM -0700, Kees Cook wrote:
> > On Thu, Apr 25, 2024 at 12:41:41AM +0200, Peter Zijlstra wrote:
> > > On Wed, Apr 24, 2024 at 12:17:34PM -0700, Kees Cook wrote:
> > > 
> > > > @@ -82,7 +83,7 @@ static __always_inline bool arch_atomic_add_negative(int i, atomic_t *v)
> > > >  
> > > >  static __always_inline int arch_atomic_add_return(int i, atomic_t *v)
> > > >  {
> > > > -	return i + xadd(&v->counter, i);
> > > > +	return wrapping_add(int, i, xadd(&v->counter, i));
> > > >  }
> > > >  #define arch_atomic_add_return arch_atomic_add_return
> > > 
> > > this is going to get old *real* quick :-/
> > > 
> > > This must be the ugliest possible way to annotate all this, and then
> > > litter the kernel with all this... urgh.
> > 
> > I'm expecting to have explicit wrapping type annotations soon[1], but for
> > the atomics, it's kind of a wash on how intrusive the annotations get. I
> > had originally wanted to mark the function (as I did in other cases)
> > rather than using the helper, but Mark preferred it this way. I'm happy
> > to do whatever! :)
> > 
> > -Kees
> > 
> > [1] https://github.com/llvm/llvm-project/pull/86618
> 
> This is arse-about-face. Signed stuff wraps per -fno-strict-overflow.
> We've been writing code for years under that assumption.

Right, which is why this is going to take time to roll out. :) What we
were really doing with -fno-strict-overflow was getting rid of undefined
behavior. That was really really horrible; we don't need the compiler
hallucinating.

> You want to mark the non-wrapping case.

What we want is lack of ambiguity. Having done these kinds of things in
the kernel for a while now, I have strong evidence that we get much better
results with the "fail safe" approach, but start by making it non-fatal.
That way we get full coverage, but we don't melt the world for anyone
that doesn't want it, and we can shake things out over a few years. For
example, it has worked well for CONFIG_FORTIFY, CONFIG_UBSAN_BOUNDS,
KCFI, etc.

The riskier condition is having something wrap when it wasn't expected
(e.g. allocations, pointer offsets, etc), so we start by defining our
regular types as non-wrapping, and annotate the wrapping types (or
specific calculations or functions).

For signed types in particular, wrapping is overwhelmingly the
uncommon case, so from a purely "how much annotations is needed"
perspective, marking wrapping is also easiest. Yes, there are cases of
expected wrapping, but we'll track them all down and get them marked
unambiguously. One thing on the short list is atomics, so here we are. :)

-Kees

-- 
Kees Cook