Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 4 Aug 2020 12:23:03 -0700
From: Kees Cook <>
To: Rasmus Villemoes <>
Cc: Jason Gunthorpe <>, Leon Romanovsky <>,
	"Gustavo A. R. Silva" <>,
	Matthew Wilcox <>,,
Subject: Re: [RFC] saturate check_*_overflow() output?

On Tue, Aug 04, 2020 at 08:11:45AM +0200, Rasmus Villemoes wrote:
> On 03/08/2020 20.29, Kees Cook wrote:
> > Hi,
> > 
> > I wonder if we should explicitly saturate the output of the overflow
> > helpers as a side-effect of overflow detection? 
> Please no.

I'm entirely on the fence about this, so I'm fine with that answer. (And
I see your PS about why -- thanks!)

> (That way the output
> > is never available with a "bad" value, if the caller fails to check the
> > result or forgets that *d was written...) since right now, *d will hold
> > the wrapped value.
> Exactly. I designed the fallback ones so they would have the same
> semantics as when using gcc's __builtin_* - though with the "all
> operands have same type" restriction, since it would be completely
> unwieldy to handle stuff like (s8) + (u64) -> (s32) in macros.

Right -- a totally sane requirement. :)

> > Also, if we enable arithmetic overflow detection sanitizers, we're going
> > to trip over the fallback implementation (since it'll wrap and then do
> > the overflow test in the macro).
> Huh? The fallback code only ever uses unsigned arithmetic, precisely to
> avoid triggering such warnings. Or are you saying there are some
> sanitizers out there which also warn for, say, (~0u) + 1u? Yes,
> detecting overflow/underflow for a (s32)-(s32)->(s32) without relying on
> -fwrapv is a bit messy, but it's done and AFAIK works just fine even
> with UBSAN enabled.

GCC only has a signed overflow sanitizer. Clang has signed and unsigned.
Dealing with -fwrapv is yet another exercise.

And I can solve this differently, too, with a static inline helper that does
basic mul and carries a no-sanitize attribute.

> What we might do, to deal with the "caller fails to check the result",
> is to add a
> static inline bool __must_check must_check_overflow(bool b) { return
> unlikely(b); }
> and wrap all the final "did it overflow" results in that one - perhaps
> also for the __builtin_* cases, I don't know if those are automatically
> equipped with that attribute. [I also don't know if gcc propagates
> likely/unlikely out to the caller, but it shouldn't hurt to have it
> there and might improve code gen if it does.]

(What is the formal name for the ({ ...; return_value; }) C construct?)

Will that work as a macro return value? If so, that's extremely useful.

> PS: Another reason not to saturate is that there are two extreme values,
> and choosing between them makes the code very messy (especially when
> using the __builtins). 5u-10u should saturate to 0u, not UINT_MAX, and
> even for for underflowing a signed computation like INT_MIN + (-7); it
> makes no sense for that to saturate to INT_MAX.

Ah, gotcha.

Kees Cook

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.