On Tue, Jul 27, 2021 at 03:43:27PM -0700, Nick Desaulniers wrote:

On Tue, Jul 27, 2021 at 2:17 PM Kees Cook [off-list ref] wrote:

quoted

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.)

quoted

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/ (local)

quoted

[...]
+#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/ (local)

quoted

[...]
  * @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.

quoted

[...]
 #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.

quoted

[...]
+// 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/ (local)

-Kees

-- 
Kees Cook