lkml.org 
[lkml]   [2021]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 34/64] fortify: Detect struct member overflows in memcpy() at compile-time
    On Tue, Jul 27, 2021 at 03:43:27PM -0700, Nick Desaulniers wrote:
    > On Tue, Jul 27, 2021 at 2:17 PM Kees Cook <keescook@chromium.org> wrote:
    > >
    > > To accelerate the review of potential run-time false positives, it's
    > > also worth noting that it is possible to partially automate checking
    > > by examining memcpy() buffer argument fields to see if they have
    > > a neighboring. It is reasonable to expect that the vast majority of
    >
    > a neighboring...field?

    Whoops, sorry, this should say "array member". I've fixed this to read:

    To accelerate the review of potential run-time false positives, it's
    also worth noting that it is possible to partially automate checking
    by examining the memcpy() buffer argument to check for the destination
    struct member having a neighboring array member. It is reasonable to
    expect that the vast majority of run-time false positives would look like
    the already evaluated and fixed compile-time false positives, where the
    most common pattern is neighboring arrays. (And, FWIW, several of the
    compile-time fixes were actual bugs.)

    > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
    > > index 7e67d02764db..5e79e626172b 100644
    > > --- a/include/linux/fortify-string.h
    > > +++ b/include/linux/fortify-string.h
    > > @@ -2,13 +2,17 @@
    > > #ifndef _LINUX_FORTIFY_STRING_H_
    > > #define _LINUX_FORTIFY_STRING_H_
    > >
    > > +#include <linux/bug.h>
    >
    > What are you using from linux/bug.h here?

    Thanks; yes, that should have been added in patch 64, when the WARN_ONCE()
    use is introduced:
    https://lore.kernel.org/lkml/20210727205855.411487-65-keescook@chromium.org/

    > > [...]
    > > +#define __fortify_memcpy_chk(p, q, size, p_size, q_size, \
    > > + p_size_field, q_size_field, op) ({ \
    > > + size_t __fortify_size = (size_t)(size); \
    > > + fortify_memcpy_chk(__fortify_size, p_size, q_size, \
    > > + p_size_field, q_size_field, #op); \
    > > + __underlying_##op(p, q, __fortify_size); \
    > > +})
    > > +
    > > +/*
    > > + * __builtin_object_size() must be captured here to avoid evaluating argument
    > > + * side-effects further into the macro layers.
    > > + */
    > > +#define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
    > > + __builtin_object_size(p, 0), __builtin_object_size(q, 0), \
    > > + __builtin_object_size(p, 1), __builtin_object_size(q, 1), \
    > > + memcpy)
    >
    > Are there other macro expansion sites for `__fortify_memcpy_chk`,
    > perhaps later in this series? I don't understand why `memcpy` is
    > passed as `func` to `fortify_panic()` rather than continuing to use
    > `__func__`?

    Yes, memmove() follows exactly the same pattern. Rather than refactoring
    the declaration in that patch, this felt cleaner.
    https://lore.kernel.org/lkml/20210727205855.411487-36-keescook@chromium.org/

    > > [...]
    > > * @count: The number of bytes to copy
    > > * @pad: Character to use for padding if space is left in destination.
    > > */
    > > -static inline void memcpy_and_pad(void *dest, size_t dest_len,
    > > - const void *src, size_t count, int pad)
    > > +static __always_inline void memcpy_and_pad(void *dest, size_t dest_len,
    > > + const void *src, size_t count,
    > > + int pad)
    >
    > Why __always_inline here?

    Without it, we run the risk of it being made out of line, and
    potentially losing access to the __builtin_object_size() checking of
    arguments. Though given some of the Clang bugs, it's possible this needs
    to be strictly converted into a macro.

    > > [...]
    > > #ifdef CONFIG_FORTIFY_SOURCE
    > > +/* These are placeholders for fortify compile-time warnings. */
    > > +void __read_overflow2_field(void) { }
    > > +EXPORT_SYMBOL(__read_overflow2_field);
    > > +void __write_overflow_field(void) { }
    > > +EXPORT_SYMBOL(__write_overflow_field);
    > > +
    >
    > Don't we rely on these being undefined for Clang to produce a linkage
    > failure (until https://reviews.llvm.org/D106030 has landed)? By
    > providing a symbol definition we can link against, I don't think
    > __compiletime_{warning|error} will warn at all with Clang?

    This was intentional because I explicitly do not want to break the build
    for new warnings, and there is no way currently for Clang to _warn_
    (rather than fail to link). This could be adjusted to break only Clang's
    builds, but at this point, it seemed best.

    > > [...]
    > > +// SPDX-License-Identifier: GPL-2.0-only
    > > +#define TEST \
    > > + memcpy(instance.buf, large, sizeof(instance.buf) + 1)
    > > +
    > > +#include "test_fortify.h"
    > > --
    >
    > I haven't read the whole series yet, but I assume test_fortify.h was
    > provided earlier in the series?

    Yup, it's part of the compile-time tests in patch 32:
    https://lore.kernel.org/lkml/20210727205855.411487-33-keescook@chromium.org/

    -Kees

    --
    Kees Cook

    \
     
     \ /
      Last update: 2021-07-28 03:48    [W:2.322 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site