Re: [PATCH 4/4] fortify: Use __builtin_dynamic_object_size() when available

[Siddhesh Poyarekar]

On 2022-09-21 23:33, Kees Cook wrote:
> On Wed, Sep 21, 2022 at 07:43:17AM -0400, Siddhesh Poyarekar wrote:
> > On 2022-09-20 15:22, Kees Cook wrote:
> > > Since the commits starting with c37495d6254c ("slab: add __alloc_size
> > > attributes for better bounds checking"), the compilers have runtime
> > > allocation size hints available in some places. This was immediately
> > > available to CONFIG_UBSAN_BOUNDS, but CONFIG_FORTIFY_SOURCE needed
> > > updating to explicitly make use the hints via the associated
> > > __builtin_dynamic_object_size() helper. Detect and use the builtin when
> > > it is available, increasing the accuracy of the mitigation. When runtime
> > > sizes are not available, __builtin_dynamic_object_size() falls back to
> > > __builtin_object_size(), leaving the existing bounds checking unchanged.
> >
> > I don't know yet what the overhead is for __builtin_dynamic_object_size vs
> > __builtin_object_size, were you able to measure it somehow for the kernel?
> > If there's a significant tradeoff, it may make sense to provide a user
> > override.
>
> So far I've not seen any measurable performance difference, but I just
> may not be creative enough yet.
>
> So far, the tunable is building a kernel with or without FORTIFY_SOURCE
> and UBSAN_BOUNDS. :)

The overhead should only be noticeable in, e.g. fortified calls inside a 
hot loop.  In theory expressions to compute sizes could be arbitrarily 
complex, but I haven't seen any cases yet that were large enough to be a 
bother.  I reckon if we find such use cases, it'll be in the kernel but 
even then it may not be worth downgrading fortification across all 
sources just for those specific hot paths.  So I agree, the user 
override isn't critically necessary.

FWIW,

Reviewed-by: Siddhesh Poyarekar <siddhesh@xxxxxxxxxx>

Thanks,
Sid