In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
The it_present member of struct ieee80211_radiotap_header is treated as a
flexible array (multiple u32s can be conditionally present). In order for
memcpy() to reason (or really, not reason) about the size of operations
against this struct, use of bytes beyond it_present need to be treated
as part of the flexible array. Add a union/struct to contain the new
"bitmap" member, for use with trailing presence bitmaps and arguments.
Additionally improve readability in the iterator code which walks
through the bitmaps and arguments.
Signed-off-by: Kees Cook <redacted>
---
include/net/ieee80211_radiotap.h | 24 ++++++++++++++++++++----
net/mac80211/rx.c | 2 +-
net/wireless/radiotap.c | 5 ++---
3 files changed, 23 insertions(+), 8 deletions(-)
@@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {*/__le16it_len;-/**-*@it_present:(first)presentword-*/-__le32it_present;+union{+/**+*@it_present:(first)presentword+*/+__le32it_present;++struct{+/* The compiler makes it difficult to overlap+*aflex-arraywithanexistingsingleton,+*sowe'reforcedtoaddanemptynamed+*variablehere.+*/+struct{}__unused;++/**+*@bitmap:allpresencebitmaps+*/+__le32bitmap[];+};+};}__packed;/* version is always 0 */
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() around members addr1, addr2, and addr3 in struct
rtl_80211_hdr_4addr, and members qui, qui_type, qui_subtype, version,
and ac_info in struct ieee80211_qos_information_element, so they can be
referenced together. This will allow memcpy() and sizeof() to more easily
reason about sizes, improve readability, and avoid future warnings about
writing beyond the end of addr1 and qui. Additionally replace zero sized
arrays with flexible arrays in struct ieee_param.
"pahole" shows no size nor member offset changes to struct
rtl_80211_hdr_4addr nor struct ieee80211_qos_information_element. "objdump
-d" shows no meaningful object code changes (i.e. only source line number
induced differences and optimizations).
Signed-off-by: Kees Cook <redacted>
---
.../staging/rtl8192u/ieee80211/ieee80211.h | 24 +++++++++++--------
.../rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 3 ++-
.../staging/rtl8192u/ieee80211/ieee80211_rx.c | 8 +++----
3 files changed, 20 insertions(+), 15 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field array bounds checking for memcpy(), memmove(), and memset(),
avoid intentionally writing across neighboring fields.
Use struct_group() in struct txpd around members tx_dest_addr_high
and tx_dest_addr_low so they can be referenced together. This will
allow memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end
of tx_dest_addr_high.
"pahole" shows no size nor member offset changes to struct txpd.
"objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/wireless/marvell/libertas_tf/libertas_tf.h | 10 ++++++----
drivers/net/wireless/marvell/libertas_tf/main.c | 3 ++-
2 files changed, 8 insertions(+), 5 deletions(-)
@@ -268,10 +268,12 @@ struct txpd {__le32tx_packet_location;/* Tx packet length */__le16tx_packet_length;-/* First 2 byte of destination MAC address */-u8tx_dest_addr_high[2];-/* Last 4 byte of destination MAC address */-u8tx_dest_addr_low[4];+struct_group(tx_dest_addr,+/* First 2 byte of destination MAC address */+u8tx_dest_addr_high[2];+/* Last 4 byte of destination MAC address */+u8tx_dest_addr_low[4];+);/* Pkt Priority */u8priority;/* Pkt Trasnit Power control */
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use flexible arrays instead of zero-element arrays (which look like they
are always overflowing) and split the cross-field memcpy() into two halves
that can be appropriately bounds-checked by the compiler.
"pahole" shows no size nor member offset changes to struct mlx5e_tx_wqe
nor struct mlx5e_umr_wqe. "objdump -d" shows no meaningful object
code changes (i.e. only source line number induced differences and
optimizations).
Signed-off-by: Kees Cook <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en.h | 4 ++--
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 +++-
2 files changed, 5 insertions(+), 3 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct rss_hdr around members imm_data and intr_gen,
so they can be referenced together. This will allow memcpy() and sizeof()
to more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of imm_data.
"pahole" shows no size nor member offset changes to struct rss_hdr.
"objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/ethernet/chelsio/cxgb3/sge.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct cp2112_string_report around members report,
length, type, and string, so they can be referenced together. This will
allow memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
report.
"pahole" shows no size nor member offset changes to struct
cp2112_string_report. "objdump -d" shows no meaningful object
code changes (i.e. only source line number induced differences.)
Signed-off-by: Kees Cook <redacted>
---
drivers/hid/hid-cp2112.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
@@ -129,10 +129,12 @@ struct cp2112_xfer_status_report {structcp2112_string_report{u8dummy;/* force .string to be aligned */-u8report;/* CP2112_*_STRING */-u8length;/* length in bytes of everyting after .report */-u8type;/* USB_DT_STRING */-wchar_tstring[30];/* UTF16_LITTLE_ENDIAN string */+struct_group_attr(contents,__packed,+u8report;/* CP2112_*_STRING */+u8length;/* length in bytes of everyting after .report */+u8type;/* USB_DT_STRING */+wchar_tstring[30];/* UTF16_LITTLE_ENDIAN string */+);}__packed;/* Number of times to request transfer status before giving up waiting for a
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field array bounds checking for memcpy(), memmove(), and memset(),
avoid intentionally writing across neighboring fields.
Use struct_group() in struct libipw_qos_information_element around
members qui, qui_type, qui_subtype, version, and ac_info, so they can be
referenced together. This will allow memcpy() and sizeof() to more easily
reason about sizes, improve readability, and avoid future warnings about
writing beyond the end of qui.
"pahole" shows no size nor member offset changes to struct
libipw_qos_information_element.
Additionally corrects the size in libipw_read_qos_param_element() as
it was testing the wrong structure size (it should have been struct
libipw_qos_information_element, not struct libipw_qos_parameter_info).
Signed-off-by: Kees Cook <redacted>
---
drivers/net/wireless/intel/ipw2x00/libipw.h | 12 +++++++-----
drivers/net/wireless/intel/ipw2x00/libipw_rx.c | 8 ++++----
2 files changed, 11 insertions(+), 9 deletions(-)
When commit a28a6e860c6c ("string.h: move fortified functions definitions
in a dedicated header.") moved the fortify-specific code, some helpers
were left behind. Moves the remaining fortify-specific helpers into
fortify-string.h so they're together where they're used. This requires
that any FORTIFY helper function prototypes be conditionally built to
avoid "no prototype" warnings. Additionally removes unused helpers.
Signed-off-by: Kees Cook <redacted>
---
include/linux/fortify-string.h | 7 +++++++
include/linux/string.h | 9 ---------
lib/string_helpers.c | 2 ++
3 files changed, 9 insertions(+), 9 deletions(-)
The core functions of string.c are those that may be implemented by
per-architecture functions, or overloaded by FORTIFY_SOURCE. As a
result, it needs to be built with __NO_FORTIFY. Without this, macros
will collide with function declarations. This was accidentally working
due to -ffreestanding (on some architectures). Make this deterministic
by explicitly setting __NO_FORTIFY and move all the helper functions
into string_helpers.c so that they gain the fortification coverage they
had been missing.
Signed-off-by: Kees Cook <redacted>
---
arch/s390/lib/string.c | 3 +
arch/x86/lib/string_32.c | 1 +
lib/string.c | 210 ++-------------------------------------
lib/string_helpers.c | 193 +++++++++++++++++++++++++++++++++++
4 files changed, 203 insertions(+), 204 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct vlan_ethhdr around members h_dest and
h_source, so they can be referenced together. This will allow memcpy()
and sizeof() to more easily reason about sizes, improve readability,
and avoid future warnings about writing beyond the end of h_dest.
"pahole" shows no size nor member offset changes to struct vlan_ethhdr.
"objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +-
include/linux/if_vlan.h | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() around members addr1, addr2, and addr3 in struct
ieee80211_hdr so they can be referenced together. This will allow memcpy()
and sizeof() to more easily reason about sizes, improve readability,
and avoid future warnings about writing beyond the end of addr1.
"pahole" shows no size nor member offset changes to struct ieee80211_hdr.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Signed-off-by: Kees Cook <redacted>
---
drivers/staging/rtl8723bs/core/rtw_security.c | 5 +++--
drivers/staging/rtl8723bs/core/rtw_xmit.c | 5 +++--
include/linux/ieee80211.h | 8 +++++---
net/wireless/lib80211_crypt_ccmp.c | 3 ++-
4 files changed, 13 insertions(+), 8 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() around members addr1, addr2, and addr3 in struct
rtllib_hdr_4addr, and members qui, qui_type, qui_subtype, version,
and ac_info in struct rtllib_qos_information_element, so they can be
referenced together. This will allow memcpy() and sizeof() to more easily
reason about sizes, improve readability, and avoid future warnings about
writing beyond the end of addr1 and qui.
"pahole" shows no size nor member offset changes to struct
rtllib_hdr_4addr nor struct rtllib_qos_information_element. "objdump -d"
shows no meaningful object code changes (i.e. only source line number
induced differences and optimizations).
Signed-off-by: Kees Cook <redacted>
---
drivers/staging/rtl8192e/rtllib.h | 20 ++++++++++++--------
drivers/staging/rtl8192e/rtllib_crypt_ccmp.c | 3 ++-
drivers/staging/rtl8192e/rtllib_rx.c | 8 ++++----
3 files changed, 18 insertions(+), 13 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct nig_stats around members egress_mac_pkt0_lo,
egress_mac_pkt0_hi, egress_mac_pkt1_lo, and egress_mac_pkt1_hi (and the
respective members in struct bnx2x_eth_stats), so they can be referenced
together. This will allow memcpy() and sizeof() to more easily reason
about sizes, improve readability, and avoid future warnings about writing
beyond the end of struct bnx2x_eth_stats's rx_stat_ifhcinbadoctets_hi.
"pahole" shows no size nor member offset changes to either struct.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Additionally adds BUILD_BUG_ON() to compare the separate struct group
sizes.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 7 ++++---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h | 14 ++++++++++----
2 files changed, 14 insertions(+), 7 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct mwl8k_cmd_set_key around members
key_material, tkip_tx_mic_key, and tkip_rx_mic_key so they can be
referenced together. This will allow memcpy() and sizeof() to more easily
reason about sizes, improve readability, and avoid future warnings about
writing beyond the end of key_material.
"pahole" shows no size nor member offset changes to struct
mwl8k_cmd_set_key. "objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/wireless/marvell/mwl8k.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct txpd around members tx_dest_addr_high
and tx_dest_addr_low so they can be referenced together. This will
allow memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end
of queue_id.
"pahole" shows no size nor member offset changes to struct txpd.
"objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/wireless/marvell/libertas/host.h | 10 ++++++----
drivers/net/wireless/marvell/libertas/tx.c | 5 +++--
2 files changed, 9 insertions(+), 6 deletions(-)
@@ -308,10 +308,12 @@ struct txpd {__le32tx_packet_location;/* Tx packet length */__le16tx_packet_length;-/* First 2 byte of destination MAC address */-u8tx_dest_addr_high[2];-/* Last 4 byte of destination MAC address */-u8tx_dest_addr_low[4];+struct_group(tx_dest_addr,+/* First 2 byte of destination MAC address */+u8tx_dest_addr_high[2];+/* Last 4 byte of destination MAC address */+u8tx_dest_addr_low[4];+);/* Pkt Priority */u8priority;/* Pkt Trasnit Power control */
@@ -0,0 +1,64 @@+#!/bin/sh+# SPDX-License-Identifier: GPL-2.0-only+set-e++# Argument 1: Source file to build.+IN="$1"+shift+# Extract just the filename for error messages below.+FILE="${IN##*/}"+# Extract the function name for error messages below.+FUNC="${FILE##*-}"+FUNC="${FUNC%%.*}"+# Extract the symbol to test for in build/symbol test below.+WANT="__${FILE%%-*}"++# Argument 2: Where to write the build log.+OUT="$1"+shift+TMP="${OUT}.tmp"++# Argument 3: Path to "nm" tool.+NM="$1"+shift++# Remaining arguments are: $(CC) $(c_flags)++# Skip this test, it is currently failing on all compilers.+if["$WANT$FUNC"="__write_overflow strlcpy"];then+echo"skip: unsafe ${FUNC}() usage not checked for '$WANT' in $IN">"$OUT"+exit0+fi++# Clean up temporary file at exit.+__cleanup(){+rm-f"$TMP"+}+trap__cleanupEXIT++status=+# Attempt to build a source that is expected to fail with a specific warning.+if"$@"-Werror-c"$IN"-o"$OUT".o2>"$TMP";then+# If the build succeeds, either the test has failed or the the+# warning may only happen at link time (Clang). In that case,+# make sure the expected symbol is unresolved in the symbol list.+# If so, FORTIFY is working for this case.+if!$NM-A"$OUT".o|grep-m1"\bU ${WANT}$">>"$TMP";then+status="warning: unsafe ${FUNC}() usage lacked '$WANT' symbol in $IN"+fi+else+# If the build failed, check for the warning in the stderr (gcc).+if!grep-q-m1"error:.*\b${WANT}'""$TMP";then+status="warning: unsafe ${FUNC}() usage lacked '$WANT' warning in $IN"+fi+fi++if[-n"$status"];then+# Report on failure results, including compilation warnings.+echo"$status"|tee"$OUT">&2+cat"$TMP"|tee-a"$OUT">&2+else+# Report on good results, and save any compilation output to log.+echo"ok: unsafe ${FUNC}() usage correctly detected with '$WANT' in $IN">"$OUT"+cat"$TMP">>"$OUT"+fi
Clang has never correctly compiled the FORTIFY_SOURCE defenses due to
a couple bugs:
Eliding inlines with matching __builtin_* names
https://bugs.llvm.org/show_bug.cgi?id=50322
Incorrect __builtin_constant_p() of some globals
https://bugs.llvm.org/show_bug.cgi?id=41459
In the process of making improvements to the FORTIFY_SOURCE defenses, the
first (silent) bug (coincidentally) becomes worked around, but exposes
the latter which breaks the build. As such, Clang must not be used with
CONFIG_FORTIFY_SOURCE until at least latter bug is fixed (in Clang 13),
and the fortify routines have been rearranged.
Update the Kconfig to reflect the reality of the current situation.
Signed-off-by: Kees Cook <redacted>
---
security/Kconfig | 3 +++
1 file changed, 3 insertions(+)
Since all compilers support __builtin_object_size(), and there is only
one user of __compiletime_object_size, remove it to avoid the needless
indirection. This lets Clang reason about check_copy_size() correctly.
Link: https://github.com/ClangBuiltLinux/linux/issues/1179
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Kees Cook <redacted>
---
include/linux/compiler-gcc.h | 2 --
include/linux/compiler_types.h | 4 ----
include/linux/thread_info.h | 2 +-
3 files changed, 1 insertion(+), 7 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct drm32_mga_init around members chipset, sgram,
maccess, fb_cpp, front_offset, front_pitch, back_offset, back_pitch,
depth_cpp, depth_offset, depth_pitch, texture_offset, and texture_size,
so they can be referenced together. This will allow memcpy() and sizeof()
to more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of chipset.
"pahole" shows no size nor member offset changes to struct drm32_mga_init.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Note that since this includes a UAPI header, struct_group() has been
explicitly redefined local to the header.
Signed-off-by: Kees Cook <redacted>
---
drivers/gpu/drm/mga/mga_ioc32.c | 30 ++++++++++++++------------
include/uapi/drm/mga_drm.h | 37 ++++++++++++++++++++++++---------
2 files changed, 44 insertions(+), 23 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() around members queue_id, min_bw, max_bw, tsa, pri_lvl,
and bw_weight so they can be referenced together. This will allow memcpy()
and sizeof() to more easily reason about sizes, improve readability,
and avoid future warnings about writing beyond the end of queue_id.
"pahole" shows no size nor member offset changes to struct bnxt_cos2bw_cfg.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Signed-off-by: Kees Cook <redacted>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c | 4 ++--
drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h | 14 ++++++++------
2 files changed, 10 insertions(+), 8 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Adjust memcpy() destination to be the named structure itself, rather than
the first member, allowing memcpy() to correctly reason about the size.
"objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/staging/rtl8723bs/core/rtw_mlme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Replace the existing empty member position markers "headers_start" and
"headers_end" with a struct_group(). This will allow memcpy() and sizeof()
to more easily reason about sizes, and improve readability.
"pahole" shows no size nor member offset changes to struct sk_buff.
"objdump -d" shows no no meaningful object code changes (i.e. only source
line number induced differences and optimizations.)
Signed-off-by: Kees Cook <redacted>
---
drivers/net/wireguard/queueing.h | 4 +---
include/linux/skbuff.h | 9 ++++-----
net/core/skbuff.c | 14 +++++---------
3 files changed, 10 insertions(+), 17 deletions(-)
@@ -800,11 +800,10 @@ struct sk_buff {__u8active_extensions;#endif-/* fields enclosed in headers_start/headers_end are copied+/* Fields enclosed in headers group are copied*usingasinglememcpy()in__copy_skb_header()*/-/* private: */-__u32headers_start[0];+struct_group(headers,/* public: *//* if you move pkt_type around you also must adapt those constants */
@@ -920,8 +919,8 @@ struct sk_buff {u64kcov_handle;#endif-/* private: */-__u32headers_end[0];+);/* end headers group */+/* public: *//* These elements must be at the end, see alloc_skb() for details. */
@@ -987,12 +987,10 @@ void napi_consume_skb(struct sk_buff *skb, int budget)}EXPORT_SYMBOL(napi_consume_skb);-/* Make sure a field is enclosed inside headers_start/headers_end section */+/* Make sure a field is contained by headers group */#define CHECK_SKB_FIELD(field) \-BUILD_BUG_ON(offsetof(structsk_buff,field)<\-offsetof(structsk_buff,headers_start));\-BUILD_BUG_ON(offsetof(structsk_buff,field)>\-offsetof(structsk_buff,headers_end));\+BUILD_BUG_ON(offsetof(structsk_buff,field)!=\+offsetof(structsk_buff,headers.field));\staticvoid__copy_skb_header(structsk_buff*new,conststructsk_buff*old){
@@ -1004,14 +1002,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)__skb_ext_copy(new,old);__nf_copy(new,old,false);-/* Note : this field could be in headers_start/headers_end section+/* Note : this field could be in the headers group.*Itisnotyetbecausewedonotwanttohavea16bithole*/new->queue_mapping=old->queue_mapping;-memcpy(&new->headers_start,&old->headers_start,-offsetof(structsk_buff,headers_end)--offsetof(structsk_buff,headers_start));+memcpy(&new->headers,&old->headers,sizeof(new->headers));CHECK_SKB_FIELD(protocol);CHECK_SKB_FIELD(csum);CHECK_SKB_FIELD(hash);
The use of strncpy() is considered deprecated for NUL-terminated
strings[1]. Replace strncpy() with strscpy_pad() (as it seems this case
expects the NUL padding to fill the allocation following the flexible
array). This additionally silences a warning seen when building under
-Warray-bounds:
./include/linux/fortify-string.h:38:30: warning: '__builtin_strncpy' offset 24 from the object at '__mptr' is out of the bounds of referenced subobject 'data' with type 'u8[]' {aka 'unsigned char[]'} at offset 24 [-Warray-bounds]
38 | #define __underlying_strncpy __builtin_strncpy
| ^
./include/linux/fortify-string.h:50:9: note: in expansion of macro '__underlying_strncpy'
50 | return __underlying_strncpy(p, q, size);
| ^~~~~~~~~~~~~~~~~~~~
drivers/rpmsg/qcom_glink_native.c: In function 'qcom_glink_work':
drivers/rpmsg/qcom_glink_native.c:36:5: note: subobject 'data' declared here
36 | u8 data[];
| ^~~~
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
Signed-off-by: Kees Cook <redacted>
---
drivers/rpmsg/qcom_glink_native.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Kernel code has a regular need to describe groups of members within a
structure usually when they need to be copied or initialized separately
from the rest of the surrounding structure. The generally accepted design
pattern in C is to use a named sub-struct:
struct foo {
int one;
struct {
int two;
int three;
} thing;
int four;
};
This would allow for traditional references and sizing:
memcpy(&dst.thing, &src.thing, sizeof(dst.thing));
However, doing this would mean that referencing struct members enclosed
by such named structs would always require including the sub-struct name
in identifiers:
do_something(dst.thing.three);
This has tended to be quite inflexible, especially when such groupings
need to be added to established code which causes huge naming churn.
Three workarounds exist in the kernel for this problem, and each have
other negative properties.
To avoid the naming churn, there is a design pattern of adding macro
aliases for the named struct:
#define f_three thing.three
This ends up polluting the global namespace, and makes it difficult to
search for identifiers.
Another common work-around in kernel code avoids the pollution by avoiding
the named struct entirely, instead identifying the group's boundaries using
either a pair of empty anonymous structs of a pair of zero-element arrays:
struct foo {
int one;
struct { } start;
int two;
int three;
struct { } finish;
int four;
};
struct foo {
int one;
int start[0];
int two;
int three;
int finish[0];
int four;
};
This allows code to avoid needing to use a sub-struct name for member
references within the surrounding structure, but loses the benefits of
being able to actually use such a struct, making it rather fragile. Using
these requires open-coded calculation of sizes and offsets. The efforts
made to avoid common mistakes include lots of comments, or adding various
BUILD_BUG_ON()s. Such code is left with no way for the compiler to reason
about the boundaries (e.g. the "start" object looks like it's 0 bytes
in length and is not structurally associated with "finish"), making bounds
checking depend on open-coded calculations:
if (length > offsetof(struct foo, finish) -
offsetof(struct foo, start))
return -EINVAL;
memcpy(&dst.start, &src.start, length);
However, the vast majority of places in the kernel that operate on
groups of members do so without any identification of the grouping,
relying either on comments or implicit knowledge of the struct contents,
which is even harder for the compiler to reason about, and results in
even more fragile manual sizing, usually depending on member locations
outside of the region (e.g. to copy "two" and "three", use the start of
"four" to find the size):
BUILD_BUG_ON((offsetof(struct foo, four) <
offsetof(struct foo, two)) ||
(offsetof(struct foo, four) <
offsetof(struct foo, three));
if (length > offsetof(struct foo, four) -
offsetof(struct foo, two))
return -EINVAL;
memcpy(&dst.two, &src.two, length);
And both of the prior two idioms additionally appear to write beyond the
end of the referenced struct member, forcing the compiler to ignore any
attempt to perform bounds checking.
In order to have a regular programmatic way to describe a struct
region that can be used for references and sizing, can be examined for
bounds checking, avoids forcing the use of intermediate identifiers,
and avoids polluting the global namespace, introduce the struct_group()
macro. This macro wraps the member declarations to create an anonymous
union of an anonymous struct (no intermediate name) and a named struct
(for references and sizing):
struct foo {
int one;
struct_group(thing,
int two,
int three,
);
int four;
};
if (length > sizeof(src.thing))
return -EINVAL;
memcpy(&dst.thing, &src.thing, length);
do_something(dst.three);
There are some rare cases where the resulting struct_group() needs
attributes added, so struct_group_attr() is also introduced to allow
for specifying struct attributes (e.g. __align(x) or __packed).
Co-developed-by: Keith Packard <redacted>
Signed-off-by: Keith Packard <redacted>
Signed-off-by: Kees Cook <redacted>
---
include/linux/stddef.h | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields. Wrap the target region
in a common named structure. This additionally fixes a theoretical
misalignment of the copy (since the size of "buf" changes between 64-bit
and 32-bit, but this is likely never built for 64-bit).
FWIW, I think this code is totally broken on 64-bit (which appears to
not be a "real" build configuration): it would either always fail (with
an uninitialized data->buf_size) or would cause corruption in userspace
due to the copy_to_user() in the call path against an uninitialized
data->buf value:
omap3isp_stat_request_statistics_time32(...)
struct omap3isp_stat_data data64;
...
omap3isp_stat_request_statistics(stat, &data64);
int omap3isp_stat_request_statistics(struct ispstat *stat,
struct omap3isp_stat_data *data)
...
buf = isp_stat_buf_get(stat, data);
static struct ispstat_buffer *isp_stat_buf_get(struct ispstat *stat,
struct omap3isp_stat_data *data)
...
if (buf->buf_size > data->buf_size) {
...
return ERR_PTR(-EINVAL);
}
...
rval = copy_to_user(data->buf,
buf->virt_addr,
buf->buf_size);
Regardless, additionally initialize data64 to be zero-filled to avoid
undefined behavior.
Fixes: 378e3f81cb56 ("media: omap3isp: support 64-bit version of omap3isp_stat_data")
Signed-off-by: Kees Cook <redacted>
---
drivers/media/platform/omap3isp/ispstat.c | 5 +--
include/uapi/linux/omap3isp.h | 44 +++++++++++++++++------
2 files changed, 36 insertions(+), 13 deletions(-)
This enables the run-time checking of dynamic memcpy() and memmove()
lengths, issuing a WARN when a write would exceed the size of the
target field.
Signed-off-by: Kees Cook <redacted>
---
include/linux/fortify-string.h | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
fs/btrfs/root-tree.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
net/802/hippi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), avoid intentionally writing across
neighboring fields.
Use struct_group() in struct art around members weight, and ac[0-9]_max,
so they can be referenced together. This will allow memcpy() and sizeof()
to more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of weight.
"pahole" shows no size nor member offset changes to struct art.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences).
Signed-off-by: Kees Cook <redacted>
---
.../intel/int340x_thermal/acpi_thermal_rel.c | 5 +-
.../intel/int340x_thermal/acpi_thermal_rel.h | 48 ++++++++++---------
2 files changed, 29 insertions(+), 24 deletions(-)
@@ -250,8 +250,9 @@ static int fill_art(char __user *ubuf)get_single_name(arts[i].source,art_user[i].source_device);get_single_name(arts[i].target,art_user[i].target_device);/* copy the rest int data in addition to source and target */-memcpy(&art_user[i].weight,&arts[i].weight,-sizeof(u64)*(ACPI_NR_ART_ELEMENTS-2));+BUILD_BUG_ON(sizeof(art_user[i].data)!=+sizeof(u64)*(ACPI_NR_ART_ELEMENTS-2));+memcpy(&art_user[i].data,&arts[i].data,sizeof(art_user[i].data));}if(copy_to_user(ubuf,art_user,art_len))
@@ -47,17 +49,19 @@ union art_object {struct{charsource_device[8];/* ACPI single name */chartarget_device[8];/* ACPI single name */-u64weight;-u64ac0_max_level;-u64ac1_max_level;-u64ac2_max_level;-u64ac3_max_level;-u64ac4_max_level;-u64ac5_max_level;-u64ac6_max_level;-u64ac7_max_level;-u64ac8_max_level;-u64ac9_max_level;+struct_group(data,+u64weight;+u64ac0_max_level;+u64ac1_max_level;+u64ac2_max_level;+u64ac3_max_level;+u64ac4_max_level;+u64ac5_max_level;+u64ac6_max_level;+u64ac7_max_level;+u64ac8_max_level;+u64ac9_max_level;+);};u64__data[ACPI_NR_ART_ELEMENTS];};
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct mlx5_ib_mr that should be
initialized to zero.
Signed-off-by: Kees Cook <redacted>
---
drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -644,6 +644,7 @@ struct mlx5_ib_mr {structib_umem*umem;/* This is zero'd when the MR is allocated */+struct_group(cleared,union{/* Used only while the MR is in the cache */struct{
@@ -691,12 +692,13 @@ struct mlx5_ib_mr {boolis_odp_implicit;};};+);};/* Zero the fields in the mr that are variant depending on usage */staticinlinevoidmlx5_clear_mr(structmlx5_ib_mr*mr){-memset(mr->out,0,sizeof(*mr)-offsetof(structmlx5_ib_mr,out));+memset(&mr->cleared,0,sizeof(mr->cleared));}staticinlineboolis_odp_mr(structmlx5_ib_mr*mr)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct ivhd_entry around members ext and hidh, so
they can be referenced together. This will allow memcpy() and sizeof()
to more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of ext.
"pahole" shows no size nor member offset changes to struct ivhd_entry.
"objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/iommu/amd/init.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
The old code seems to be doing the wrong thing: starting from not the
first member, but sized for the whole struct. Which is correct?
---
drivers/net/ethernet/qlogic/qede/qede_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
To avoid a run-time false positive in the stricter FORTIFY_SOURCE
memcpy() checks, split the memcpy() into the struct and the data.
Additionally switch the data member to a flexible array to follow
modern language conventions.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/wireless/intel/iwlwifi/fw/file.h | 2 +-
drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
net/dccp/trace.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct. Additionally split up a later
field-spanning memset() so that memset() can reason about the size.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/wireless/ath/ath11k/hal_rx.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Clear trailing padding bytes using the new helper so that memset()
doesn't get confused about writing "past the end" of the last struct
member. There is no change to the resulting machine code.
Signed-off-by: Kees Cook <redacted>
---
net/xfrm/xfrm_policy.c | 4 +---
net/xfrm/xfrm_user.c | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct. Additionally, since everything
appears to perform a roundup (including allocation), just change the
size of the struct itself and add a build-time check to validate the
expected size.
Signed-off-by: Kees Cook <redacted>
---
drivers/infiniband/hw/cxgb4/cm.c | 5 +++--
drivers/net/ethernet/chelsio/cxgb4/t4_msg.h | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct kone_mouse_event that should
be initialized to zero.
Signed-off-by: Kees Cook <redacted>
---
drivers/hid/hid-roccat-kone.c | 2 +-
drivers/hid/hid-roccat-kone.h | 12 +++++++-----
2 files changed, 8 insertions(+), 6 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Note that the common helper, ieee80211_tx_info_clear_status(), does NOT
clear ack_signal, but the open-coded versions do. All three perform
checks that the ack_signal position hasn't changed, though.
Signed-off-by: Kees Cook <redacted>
---
Should these each be clearing the same region? Because they're currently not.
---
drivers/net/wireless/ath/carl9170/tx.c | 4 +---
drivers/net/wireless/intersil/p54/txrx.c | 4 +---
include/net/mac80211.h | 4 +---
3 files changed, 3 insertions(+), 9 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct flowi4, struct ipv4hdr, and struct ipv6hdr
around members saddr and daddr, so they can be referenced together. This
will allow memcpy() and sizeof() to more easily reason about sizes,
improve readability, and avoid future warnings about writing beyond the
end of saddr.
"pahole" shows no size nor member offset changes to struct flowi4.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences.)
Note that since this is a UAPI header, struct_group() has been open
coded.
Signed-off-by: Kees Cook <redacted>
---
include/net/flow.h | 6 ++++--
include/uapi/linux/if_ether.h | 12 ++++++++++--
include/uapi/linux/ip.h | 12 ++++++++++--
include/uapi/linux/ipv6.h | 12 ++++++++++--
net/core/flow_dissector.c | 10 ++++++----
net/ipv4/ip_output.c | 6 ++----
6 files changed, 42 insertions(+), 16 deletions(-)
@@ -81,8 +81,10 @@ struct flowi4 {#define flowi4_multipath_hash __fl_common.flowic_multipath_hash/* (saddr,daddr) must be grouped, same order as in IP header */-__be32saddr;-__be32daddr;+struct_group(addrs,+__be32saddr;+__be32daddr;+);unionflowi_uliuli;#define fl4_sport uli.ports.sport
@@ -444,10 +444,8 @@ EXPORT_SYMBOL(ip_output);*/staticvoidip_copy_addrs(structiphdr*iph,conststructflowi4*fl4){-BUILD_BUG_ON(offsetof(typeof(*fl4),daddr)!=-offsetof(typeof(*fl4),saddr)+sizeof(fl4->saddr));-memcpy(&iph->saddr,&fl4->saddr,-sizeof(fl4->saddr)+sizeof(fl4->daddr));+BUILD_BUG_ON(sizeof(iph->addrs)!=sizeof(fl4->addrs));+memcpy(&iph->addrs,&fl4->addrs,sizeof(fl4->addrs));}/* Note: skb->sk can be different from sk, in case of tunnels */
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct rt6_info that should be
initialized to zero.
Signed-off-by: Kees Cook <redacted>
---
include/net/ip6_fib.h | 30 ++++++++++++++++--------------
net/ipv6/route.c | 4 +---
2 files changed, 17 insertions(+), 17 deletions(-)
@@ -205,20 +205,22 @@ struct fib6_info {structrt6_info{structdst_entrydst;-structfib6_info__rcu*from;-intsernum;--structrt6keyrt6i_dst;-structrt6keyrt6i_src;-structin6_addrrt6i_gateway;-structinet6_dev*rt6i_idev;-u32rt6i_flags;--structlist_headrt6i_uncached;-structuncached_list*rt6i_uncached_list;--/* more non-fragment space at head required */-unsignedshortrt6i_nfheader_len;+struct_group(init,+structfib6_info__rcu*from;+intsernum;++structrt6keyrt6i_dst;+structrt6keyrt6i_src;+structin6_addrrt6i_gateway;+structinet6_dev*rt6i_idev;+u32rt6i_flags;++structlist_headrt6i_uncached;+structuncached_list*rt6i_uncached_list;++/* more non-fragment space at head required */+unsignedshortrt6i_nfheader_len;+);};structfib6_result{
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct x86_emulate_ctxt that should
be initialized to zero.
Signed-off-by: Kees Cook <redacted>
---
arch/x86/kvm/emulate.c | 3 +--
arch/x86/kvm/kvm_emulate.h | 19 +++++++++++--------
2 files changed, 12 insertions(+), 10 deletions(-)
@@ -341,14 +341,17 @@ struct x86_emulate_ctxt {*therestareinitializedunconditionallyinx86_decode_insn*orelsewhere*/-boolrip_relative;-u8rex_prefix;-u8lock_prefix;-u8rep_prefix;-/* bitmaps of registers in _regs[] that can be read */-u32regs_valid;-/* bitmaps of registers in _regs[] that have been written */-u32regs_dirty;+struct_group(decode_cache,+boolrip_relative;+u8rex_prefix;+u8lock_prefix;+u8rep_prefix;+/* bitmaps of registers in _regs[] that can be read */+u32regs_valid;+/* bitmaps of registers in _regs[] that have been written */+u32regs_dirty;+);+/* modrm */u8modrm;u8modrm_mod;
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct stats_reply_data that should
be initialized, which can now be done in a single memset() call.
Signed-off-by: Kees Cook <redacted>
---
net/ethtool/stats.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
@@ -117,10 +119,7 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,/* Mark all stats as unset (see ETHTOOL_STAT_NOT_SET) to prevent them*frombeingreportedtouserspaceincasedriverdidnotsetthem.*/-memset(&data->phy_stats,0xff,sizeof(data->phy_stats));-memset(&data->mac_stats,0xff,sizeof(data->mac_stats));-memset(&data->ctrl_stats,0xff,sizeof(data->ctrl_stats));-memset(&data->rmon_stats,0xff,sizeof(data->rmon_stats));+memset(&data->stats,0xff,sizeof(data->stats));if(test_bit(ETHTOOL_STATS_ETH_PHY,req_info->stat_mask)&&dev->ethtool_ops->get_eth_phy_stats)
With the recent fixes for flexible arrays and expanded FORTIFY_SOURCE
coverage, it is now possible to enable -Warray-bounds. Since both
GCC and Clang include -Warray-bounds in -Wall, we just need to stop
disabling it.
Co-developed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Kees Cook <redacted>
---
Makefile | 1 -
1 file changed, 1 deletion(-)
@@ -1089,7 +1089,6 @@ KBUILD_CFLAGS += $(call cc-disable-warning, stringop-truncation)# We'll want to enable this eventually, but it's not going away for 5.7 at leastKBUILD_CFLAGS+=$(callcc-disable-warning,zero-length-bounds)-KBUILD_CFLAGS+=$(callcc-disable-warning,array-bounds)KBUILD_CFLAGS+=$(callcc-disable-warning,stringop-overflow)# Another good warning that we'll want to enable eventually
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Replace the empty __nfct_init_offset member with a struct_group() to
mark the region of struct nf_conn that should be initialized to zero,
allowing memset() to correctly reason about the size of the write.
Signed-off-by: Kees Cook <redacted>
---
include/net/netfilter/nf_conntrack.h | 20 ++++++++++----------
net/netfilter/nf_conntrack_core.c | 4 +---
2 files changed, 11 insertions(+), 13 deletions(-)
@@ -101,24 +101,24 @@ struct nf_conn {structhlist_nodenat_bysource;#endif/* all members below initialized via memset */-struct{}__nfct_init_offset;--/* If we were expected by an expectation, this will be it */-structnf_conn*master;+struct_group(init,+/* If we were expected by an expectation, this will be it */+structnf_conn*master;#if defined(CONFIG_NF_CONNTRACK_MARK)-u_int32_tmark;+u_int32_tmark;#endif#ifdef CONFIG_NF_CONNTRACK_SECMARK-u_int32_tsecmark;+u_int32_tsecmark;#endif-/* Extensions */-structnf_ct_ext*ext;+/* Extensions */+structnf_ct_ext*ext;-/* Storage reserved for other modules, must be the last member */-unionnf_conntrack_protoproto;+/* Storage reserved for other modules, must be the last member */+unionnf_conntrack_protoproto;+);};staticinlinestructnf_conn*
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Instead of writing beyond the end of evt_struct->iu.srp.cmd, target the
upper union (evt_struct->iu.srp) instead, as that's what is being wiped.
Signed-off-by: Kees Cook <redacted>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd,returnSCSI_MLQUEUE_HOST_BUSY;/* Set up the actual SRP IU */+memset(&evt_struct->iu.srp,0x00,SRP_MAX_IU_LEN);srp_cmd=&evt_struct->iu.srp.cmd;-memset(srp_cmd,0x00,SRP_MAX_IU_LEN);srp_cmd->opcode=SRP_CMD;memcpy(srp_cmd->cdb,cmnd->cmnd,sizeof(srp_cmd->cdb));int_to_scsilun(lun,&srp_cmd->lun);
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct trace_iterator that should
be initialized to zero.
Signed-off-by: Kees Cook <redacted>
---
include/linux/trace_events.h | 26 ++++++++++++++------------
kernel/trace/trace.c | 4 +---
2 files changed, 15 insertions(+), 15 deletions(-)
@@ -101,18 +101,20 @@ struct trace_iterator {boolsnapshot;/* The below is zeroed out in pipe_read */-structtrace_seqseq;-structtrace_entry*ent;-unsignedlonglost_events;-intleftover;-intent_size;-intcpu;-u64ts;--loff_tpos;-longidx;--/* All new field here will be zeroed out in pipe_read */+struct_group(init,+structtrace_seqseq;+structtrace_entry*ent;+unsignedlonglost_events;+intleftover;+intent_size;+intcpu;+u64ts;++loff_tpos;+longidx;++/* All new field here will be zeroed out in pipe_read */+);};enumtrace_iter_flags{
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
PPTable_t
so the grouped members can be referenced together. This will allow
memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
the first member.
"pahole" shows no size nor member offset changes to any structs.
"objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/gpu/drm/amd/include/atomfirmware.h | 9 ++++++++-
.../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h | 3 ++-
drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++-
.../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++-
drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 +++---
drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 12 ++++++++----
drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 +++---
7 files changed, 28 insertions(+), 14 deletions(-)
@@ -2159,7 +2160,7 @@ struct atom_smc_dpm_info_v4_5uint32_tMvddRatio;// This is used for MVDD Vid workaround. It has 16 fractional bits (Q16.16)uint32_tBoardReserved[9];-+);};structatom_smc_dpm_info_v4_6
@@ -2168,6 +2169,7 @@ struct atom_smc_dpm_info_v4_6// section: board parametersuint32_ti2c_padding[3];// old i2c control are moved to new area+struct_group(dpm_info,uint16_tmaxvoltagestepgfx;// in mv(q2) max voltage step that smu will request. multiple steps are taken if voltage change exceeds this value.uint16_tmaxvoltagestepsoc;// in mv(q2) max voltage step that smu will request. multiple steps are taken if voltage change exceeds this value.
@@ -643,6 +643,7 @@ typedef struct {// SECTION: BOARD PARAMETERS// SVI2 Board Parameters+struct_group(v4_6,uint16_tMaxVoltageStepGfx;// In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value.uint16_tMaxVoltageStepSoc;// In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value.
@@ -728,10 +729,10 @@ typedef struct {uint32_tBoardVoltageCoeffB;// decode by /1000uint32_tBoardReserved[7];+);// Padding for MMHUB - do not modify thisuint32_tMmHubPadding[8];// SMU internal use-}PPTable_t;typedefstruct{
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Instead of writing across a field boundary with memset(), move the call
to just the array, and an explicit zeroing of the prior field.
Signed-off-by: Kees Cook <redacted>
---
drivers/macintosh/smu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct journal_sector that should be
initialized to zero.
Signed-off-by: Kees Cook <redacted>
---
drivers/md/dm-integrity.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
memcpy() is dead; long live memcpy()
tl;dr: In order to eliminate a large class of common buffer overflow
flaws that continue to persist in the kernel, have memcpy() (under
CONFIG_FORTIFY_SOURCE) perform bounds checking of the destination struct
member when they have a known size. This would have caught all of the
memcpy()-related buffer write overflow flaws identified in at least the
last three years.
Background and analysis:
While stack-based buffer overflow flaws are largely mitigated by stack
canaries (and similar) features, heap-based buffer overflow flaws continue
to regularly appear in the kernel. Many classes of heap buffer overflows
are mitigated by FORTIFY_SOURCE when using the strcpy() family of
functions, but a significant number remain exposed through the memcpy()
family of functions.
At its core, FORTIFY_SOURCE uses the compiler's __builtin_object_size()
internal[0] to determine the available size at a target address based on
the compile-time known structure layout details. It operates in two
modes: outer bounds (0) and inner bounds (1). In mode 0, the size of the
enclosing structure is used. In mode 1, the size of the specific field
is used. For example:
struct object {
u16 scalar1; /* 2 bytes */
char array[6]; /* 6 bytes */
u64 scalar2; /* 8 bytes */
u32 scalar3; /* 4 bytes */
} instance;
__builtin_object_size(instance.array, 0) == 18, since the remaining size
of the enclosing structure starting from "array" is 18 bytes (6 + 8 + 4).
__builtin_object_size(instance.array, 1) == 6, since the remaining size
of the specific field "array" is 6 bytes.
The initial implementation of FORTIFY_SOURCE used mode 0 because there
were many cases of both strcpy() and memcpy() functions being used to
write (or read) across multiple fields in a structure. For example,
this would catch this, which is writing 2 bytes beyond the end of
"instance":
memcpy(&instance.array, data, 20);
While this didn't protect against overwriting adjacent fields in a given
structure, it would at least stop overflows from reaching beyond the
end of the structure into neighboring memory, and provided a meaningful
mitigation of a subset of buffer overflow flaws. However, many desirable
targets remain within the enclosing structure (for example function
pointers).
As it happened, there were very few cases of strcpy() family functions
intentionally writing beyond the end of a string buffer. Once all known
cases were removed from the kernel, the strcpy() family was tightened[1]
to use mode 1, providing greater mitigation coverage.
What remains is switching memcpy() to mode 1 as well, but making the
switch is much more difficult because of how frustrating it can be to
find existing "normal" uses of memcpy() that expect to write (or read)
across multiple fields. The root cause of the problem is that the C
language lacks a common pattern to indicate the intent of an author's
use of memcpy(), and is further complicated by the available compile-time
and run-time mitigation behaviors.
The FORTIFY_SOURCE mitigation comes in two halves: the compile-time half,
when both the buffer size _and_ the length of the copy is known, and the
run-time half, when only the buffer size is known. If neither size is
known, there is no bounds checking possible. At compile-time when the
compiler sees that a length will always exceed a known buffer size,
a warning can be deterministically emitted. For the run-time half,
the length is tested against the known size of the buffer, and the
overflowing operation is detected. (The performance overhead for these
tests is virtually zero.)
It is relatively easy to find compile-time false-positives since a warning
is always generated. Fixing the false positives, however, can be very
time-consuming as there are hundreds of instances. While it's possible
some over-read conditions could lead to kernel memory exposures, the bulk
of the risk comes from the run-time flaws where the length of a write
may end up being attacker-controlled and lead to an overflow.
Many of the compile-time false-positives take a form similar to this:
memcpy(&instance.scalar2, data, sizeof(instance.scalar2) +
sizeof(instance.scalar3));
and the run-time ones are similar, but lack a constant expression for the
size of the copy:
memcpy(instance.array, data, length);
The former is meant to cover multiple fields (though its style has been
frowned upon more recently), but has been technically legal. Both lack
any expressivity in the C language about the author's _intent_ in a way
that a compiler can check when the length isn't known at compile time.
A comment doesn't work well because what's needed is something a compiler
can directly reason about. Is a given memcpy() call expected to overflow
into neighbors? Is it not? By using the new struct_group() macro, this
intent can be much more easily encoded.
It is not as easy to find the run-time false-positives since the code path
to exercise a seemingly out-of-bounds condition that is actually expected
may not be trivially reachable. Tightening the restrictions to block an
operation for a false positive will either potentially create a greater
flaw (if a copy is truncated by the mitigation), or destabilize the kernel
(e.g. with a BUG()), making things completely useless for the end user.
As a result, tightening the memcpy() restriction (when there is a
reasonable level of uncertainty of the number of false positives), needs
to first WARN() with no truncation. (Though any sufficiently paranoid
end-user can always opt to set the panic_on_warn=1 sysctl.) Once enough
development time has passed, the mitigation can be further intensified.
Given the potential frustrations of weeding out all the false positives
when tightening the run-time checks, it is reasonable to wonder if these
changes would actually add meaningful protection. Looking at just the
last three years, there are 23 identified flaws with a CVE that mention
"buffer overflow", and 11 are memcpy()-related buffer overflows.
(For the remaining 12: 7 are array index overflows that would be
mitigated by systems built with CONFIG_UBSAN_BOUNDS=y: CVE-2019-0145,
CVE-2019-14835, CVE-2019-14896, CVE-2019-14897, CVE-2019-14901,
CVE-2019-17666, CVE-2021-28952. 2 are miscalculated allocation
sizes which could be mitigated with memory tagging: CVE-2019-16746,
CVE-2019-2181. 1 is an iovec buffer bug maybe mitigated by memory tagging:
CVE-2020-10742. 1 is a type confusion bug mitigated by stack canaries:
CVE-2020-10942. 1 is a string handling logic bug with no mitigation I'm
aware of: CVE-2021-28972.)
At my last count on an x86_64 allmodconfig build, there are 25,018 calls
to memcpy(). With callers instrumented to report all places where the
buffer size is known but the length remains unknown (i.e. a run-time
bounds check is added), we can count how many new run-time bounds checks
are added when the destination and source arguments of memcpy() are
changed to use "mode 1" bounds checking: 1540. In addition, there were
146 new compile-time warnings to evaluate and fix.
With this it's also possible to compare the places where the known 11
memcpy() flaw overflows happened against the resulting list of potential
new bounds checks, as a measure of potential efficacy of the tightened
mitigation. Much to my surprise, horror, and delight, all 11 flaws would
have been detected by the newly added run-time bounds checks, making this
a distinctly clear mitigation improvement: 100% coverage for memcpy()
flaws, with a possible 2 orders of magnitude gain in coverage over
existing but undiscovered run-time dynamic length flaws, against only 6%
of all callers maybe gaining a false positive run-time check, with fewer
than 150 new compile-time instances needing evaluation.
Specifically these would have been mitigated:
CVE-2020-24490 https://git.kernel.org/linus/a2ec905d1e160a33b2e210e45ad30445ef26ce0e
CVE-2020-12654 https://git.kernel.org/linus/3a9b153c5591548612c3955c9600a98150c81875
CVE-2020-12653 https://git.kernel.org/linus/b70261a288ea4d2f4ac7cd04be08a9f0f2de4f4d
CVE-2019-14895 https://git.kernel.org/linus/3d94a4a8373bf5f45cf5f939e88b8354dbf2311b
CVE-2019-14816 https://git.kernel.org/linus/7caac62ed598a196d6ddf8d9c121e12e082cac3a
CVE-2019-14815 https://git.kernel.org/linus/7caac62ed598a196d6ddf8d9c121e12e082cac3a
CVE-2019-14814 https://git.kernel.org/linus/7caac62ed598a196d6ddf8d9c121e12e082cac3a
CVE-2019-10126 https://git.kernel.org/linus/69ae4f6aac1578575126319d3f55550e7e440449
CVE-2019-9500 https://git.kernel.org/linus/1b5e2423164b3670e8bc9174e4762d297990deff
no-CVE-yet https://git.kernel.org/linus/130f634da1af649205f4a3dd86cbe5c126b57914
no-CVE-yet https://git.kernel.org/linus/d10a87a3535cce2b890897914f5d0d83df669c63
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
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.)
Implementation:
Tighten the memcpy() buffer size checking to use the actual ("mode 1")
target buffer size as the bounds check instead of their enclosing
structure's ("mode 0") size. Use a common inline for memcpy() (and
memmove() in a following patch), since all the tests are the same. All new
cross-field memcpy() uses must use the struct_group() macro or similar
to target a specific range of fields, so that FORTIFY_SOURCE can reason
about the size and safety of the copy.
For run-time, the "mode 0" size checking and mitigation is left unchanged,
with "mode 1" added only to writes, and only performing a WARN() for
now. This way any missed run-time false positives can be flushed out over
the coming several development cycles, but system builders who have tested
their workloads to be WARN()-free can enable the panic_on_warn=1 sysctl
to immediately gain a mitigation against this class of buffer overflows.
For now, cross-member "mode 1" read detection at compile-time will be
limited to W=1 builds, since it is, unfortunately, very common. As the
priority is solving write overflows, read overflows can be the next
phase. Similarly, run-time cross-member "mode 1" read detection will be
added at a later time, once write false-positives have been handled.
Related classes of flaws that remain unmitigated:
- memcpy() with raw pointers (e.g. void *, char *, etc) have no good
mitigation beyond memory tagging (and even that would only protect
against inter-object overflow, not intra-object neighboring field
overflows). Some kind of "fat pointer" solution is likely needed to
gain proper size-of-buffer awareness.
- type confusion where a higher level type's allocation size does
not match the resulting cast type eventually passed to a deeper
memcpy() call where the compiler cannot see the true type. In
theory, greater static analysis could catch these.
[0] https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html
[1] https://git.kernel.org/linus/6a39e62abbafd1d58d1722f40c7d26ef379c6a2f
Signed-off-by: Kees Cook <redacted>
---
include/linux/fortify-string.h | 111 ++++++++++++++++--
include/linux/string.h | 5 +-
lib/Makefile | 3 +-
lib/string_helpers.c | 6 +
.../read_overflow2_field-memcpy.c | 5 +
.../write_overflow_field-memcpy.c | 5 +
6 files changed, 120 insertions(+), 15 deletions(-)
create mode 100644 lib/test_fortify/read_overflow2_field-memcpy.c
create mode 100644 lib/test_fortify/write_overflow_field-memcpy.c
@@ -2,13 +2,17 @@#ifndef _LINUX_FORTIFY_STRING_H_#define _LINUX_FORTIFY_STRING_H_+#include<linux/bug.h>+#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))#define __RENAME(x) __asm__(#x)voidfortify_panic(constchar*name)__noreturn__cold;void__read_overflow(void)__compiletime_error("detected read beyond size of object (1st parameter)");void__read_overflow2(void)__compiletime_error("detected read beyond size of object (2nd parameter)");+void__read_overflow2_field(void)__compiletime_warning("detected read beyond size of field (2nd parameter); maybe use struct_group()?");void__write_overflow(void)__compiletime_error("detected write beyond size of object (1st parameter)");+void__write_overflow_field(void)__compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?");#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)externvoid*__underlying_memchr(constvoid*p,intc,__kernel_size_tsize)__RENAME(memchr);
@@ -182,22 +186,105 @@ __FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size)return__underlying_memset(p,c,size);}-__FORTIFY_INLINEvoid*memcpy(void*p,constvoid*q,__kernel_size_tsize)+/*+*Tomakesurethecompilercanenforceprotectionagainstbufferoverflows,+*memcpy(),memmove(),andmemset()mustnotbeusedbeyondindividual+*structmembers.Ifyouneedtocopyacrossmultiplemembers,pleaseuse+*struct_group()tocreateanamedmirrorofananonymousstructunion.+*(e.g.seestructsk_buff.)+*+*Mitigationcoverage+*Boundscheckingat:+*+-------+-------+-------+-------++*|Compiletime|Runtime|+*memcpy()argumentsizes:|write|read|write|read|+*+-------+-------+-------+-------++*memcpy(known,known,constant)|y|y|n/a|n/a|+*memcpy(unknown,known,constant)|n|y|V|n/a|+*memcpy(known,unknown,constant)|y|n|n/a|V|+*memcpy(unknown,unknown,constant)|n|n|V|V|+*memcpy(known,known,dynamic)|n|n|b|B|+*memcpy(unknown,known,dynamic)|n|n|V|B|+*memcpy(known,unknown,dynamic)|n|n|b|V|+*memcpy(unknown,unknown,dynamic)|n|n|V|V|+*+-------+-------+-------+-------++*+*y=deterministiccompile-timeboundschecking+*n=cannotdodeterministiccompile-timeboundschecking+*n/a=norun-timeboundscheckingneededsincecompile-timedeterministic+*b=performrun-timeboundschecking+*B=canperformrun-timeboundschecking,butcurrentunenforced+*V=vulnerabletorun-timeoverflow+*+*/+__FORTIFY_INLINEvoidfortify_memcpy_chk(__kernel_size_tsize,+constsize_tp_size,+constsize_tq_size,+constsize_tp_size_field,+constsize_tq_size_field,+constchar*func){-size_tp_size=__builtin_object_size(p,0);-size_tq_size=__builtin_object_size(q,0);-if(__builtin_constant_p(size)){-if(p_size<size)+/*+*Lengthargumentisaconstantexpression,sowe+*canperformcompile-timeboundscheckingwhere+*buffersizesareknown.+*/++/* Error when size is larger than enclosing struct. */+if(p_size>p_size_field&&p_size<size)__write_overflow();-if(q_size<size)+if(q_size>q_size_field&&q_size<size)__read_overflow2();++/* Warn when write size argument larger than dest field. */+if(p_size_field<size)+__write_overflow_field();+/*+*Warnforsourcefieldover-readwhenbuildingwithW=1+*orwhenanover-writehappened,sobothcanbefixedat+*thesametime.+*/+if((IS_ENABLED(KBUILD_EXTRA_WARN1)||p_size_field<size)&&+q_size_field<size)+__read_overflow2_field();}-if(p_size<size||q_size<size)-fortify_panic(__func__);-return__underlying_memcpy(p,q,size);+/*+*Atthispoint,lengthargumentmaynotbeaconstantexpression,+*sorun-timeboundscheckingcanbedonewherebuffersizesare+*known.(Thisisnotan"else"becausetheabovechecksmayonly+*becompile-timewarnings,andwewanttostillwarnforrun-time+*overflows.)+*/++/*+*Alwaysstopaccessesbeyondthestructthatcontainsthe+*field,whenthebuffer'sremainingsizeisknown.+*(The-1testistooptimizeawaycheckswherethebuffer+*lengthsareunknown.)+*/+if((p_size!=(size_t)(-1)&&p_size<size)||+(q_size!=(size_t)(-1)&&q_size<size))+fortify_panic(func);}+#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()mustbecapturedheretoavoidevaluatingargument+*side-effectsfurtherintothemacrolayers.+*/+#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)+__FORTIFY_INLINEvoid*memmove(void*p,constvoid*q,__kernel_size_tsize){size_tp_size=__builtin_object_size(p,0);
@@ -277,27 +364,27 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)return__real_kmemdup(p,size,gfp);}-/* defined after fortified strlen and memcpy to reuse them */+/* Defined after fortified strlen to reuse it. */__FORTIFY_INLINEchar*strcpy(char*p,constchar*q){size_tp_size=__builtin_object_size(p,1);size_tq_size=__builtin_object_size(q,1);size_tsize;+/* If neither buffer size is known, immediately give up. */if(p_size==(size_t)-1&&q_size==(size_t)-1)return__underlying_strcpy(p,q);size=strlen(q)+1;/* test here to use the more stringent object size */if(p_size<size)fortify_panic(__func__);-memcpy(p,q,size);+__underlying_memcpy(p,q,size);returnp;}/* Don't use these outside the FORITFY_SOURCE implementation */#undef __underlying_memchr#undef __underlying_memcmp-#undef __underlying_memcpy#undef __underlying_memmove#undef __underlying_memset#undef __underlying_strcat
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct hfa384x_tx_frame around members
frame_control, duration_id, address[1-4], and sequence_control, so they
can be referenced together. This will allow memcpy() and sizeof() to
more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of frame_control.
"pahole" shows no size nor member offset changes to struct
hfa384x_tx_frame. "objdump -d" shows no meaningful object code changes
(i.e. only source line number induced differences.)
Signed-off-by: Kees Cook <redacted>
---
drivers/staging/wlan-ng/hfa384x.h | 16 +++++++++-------
drivers/staging/wlan-ng/hfa384x_usb.c | 4 +++-
2 files changed, 12 insertions(+), 8 deletions(-)
@@ -476,13 +476,15 @@ struct hfa384x_tx_frame {/*-- 802.11 Header Information --*/-u16frame_control;-u16duration_id;-u8address1[6];-u8address2[6];-u8address3[6];-u16sequence_control;-u8address4[6];+struct_group(p80211,+u16frame_control;+u16duration_id;+u8address1[6];+u8address2[6];+u8address3[6];+u16sequence_control;+u8address4[6];+);__le16data_len;/* little endian format *//*-- 802.3 Header Information --*/
@@ -2516,7 +2516,9 @@ int hfa384x_drvr_txframe(struct hfa384x *hw, struct sk_buff *skb,cpu_to_le16s(&hw->txbuff.txfrm.desc.tx_control);/* copy the header over to the txdesc */-memcpy(&hw->txbuff.txfrm.desc.frame_control,p80211_hdr,+BUILD_BUG_ON(sizeof(hw->txbuff.txfrm.desc.p80211)!=+sizeof(unionp80211_hdr));+memcpy(&hw->txbuff.txfrm.desc.p80211,p80211_hdr,sizeof(unionp80211_hdr));/* if we're using host WEP, increase size by IV+ICV */
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
drivers/infiniband/hw/mthca/mthca_mr.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct cm4000_dev that should be
initialized to zero.
Signed-off-by: Kees Cook <redacted>
---
drivers/char/pcmcia/cm4000_cs.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
@@ -116,8 +116,9 @@ struct cm4000_dev {wait_queue_head_tatrq;/* wait for ATR valid */wait_queue_head_treadq;/* used by write to wake blk.read */-/* warning: do not move this fields.+/* warning: do not move this struct group.*initialisingtozerodependsonit-seeZERO_DEVbelow.*/+struct_group(init,unsignedcharatr_csum;unsignedcharatr_len_retry;unsignedshortatr_len;
@@ -140,12 +141,10 @@ struct cm4000_dev {structtimer_listtimer;/* used to keep monitor running */intmonitor_running;+);};-#define ZERO_DEV(dev) \-memset(&dev->atr_csum,0,\-sizeof(structcm4000_dev)-\-offsetof(structcm4000_dev,atr_csum))+#define ZERO_DEV(dev) memset(&dev->init, 0, sizeof(dev->init))staticstructpcmcia_device*dev_table[CM4000_MAX_DEV];staticstructclass*cmm_class;
To enable FORTIFY_SOURCE support for Clang, the kernel must work around
a pair of bugs, related to Clang's inlining.
Change all the fortified APIs into macros with different inline names to
bypass Clang's broken inline-of-a-builtin detection:
https://bugs.llvm.org/show_bug.cgi?id=50322
Lift all misbehaving __builtin_object_size() calls into the macros to
bypass Clang's broken __builtin_object_size() arguments-of-an-inline
visibility:
https://github.com/ClangBuiltLinux/linux/issues/1401
Thankfully, due to how the inlining already behaves in GCC, this change
has no effect on GCC builds, but allows Clang to finally gain full
FORTIFY coverage.
However, because of a third bug which had no work-arounds, FORTIFY_SOURCE
will only work with Clang version 13 and later. Update the Kconfig to
reflect the new requirements.
Signed-off-by: Kees Cook <redacted>
---
include/linux/fortify-string.h | 33 +++++++++++++++++++++------------
security/Kconfig | 2 +-
2 files changed, 22 insertions(+), 13 deletions(-)
@@ -112,12 +113,15 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)/* defined after fortified strnlen to reuse it */externssize_t__real_strscpy(char*,constchar*,size_t)__RENAME(strscpy);-__FORTIFY_INLINEssize_tstrscpy(char*p,constchar*q,size_tsize)+#define strscpy(p, q, s) __fortify_strscpy(p, q, s, \+__builtin_object_size(p,1),\+__builtin_object_size(q,1))+__FORTIFY_INLINEssize_t__fortify_strscpy(char*p,constchar*q,+size_tsize,+constsize_tp_size,+constsize_tq_size){size_tlen;-/* Use string size rather than possible enclosing struct size. */-size_tp_size=__builtin_object_size(p,1);-size_tq_size=__builtin_object_size(q,1);/* If we cannot get size of p and q default to call strscpy. */if(p_size==(size_t)-1&&q_size==(size_t)-1)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct fw_eth_tx_pkt_vm_wr around members ethmacdst,
ethmacsrc, ethtype, and vlantci, so they can be referenced together. This
will allow memcpy() and sizeof() to more easily reason about sizes,
improve readability, and avoid future warnings about writing beyond the
end of ethmacdst.
"pahole" shows no size nor member offset changes to struct
fw_eth_tx_pkt_vm_wr. "objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/ethernet/chelsio/cxgb4/sge.c | 8 +++++---
drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h | 10 ++++++----
drivers/net/ethernet/chelsio/cxgb4vf/sge.c | 7 ++-----
3 files changed, 13 insertions(+), 12 deletions(-)
@@ -1924,7 +1926,7 @@ static netdev_tx_t cxgb4_vf_eth_xmit(struct sk_buff *skb,wr->equiq_to_len16=cpu_to_be32(wr_mid);wr->r3[0]=cpu_to_be32(0);wr->r3[1]=cpu_to_be32(0);-skb_copy_from_linear_data(skb,(void*)wr->ethmacdst,fw_hdr_copy_len);+skb_copy_from_linear_data(skb,&wr->firmware,fw_hdr_copy_len);end=(u64*)wr+flits;/* If this is a Large Send Offload packet we'll put in an LSO CPL
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
drivers/hwtracing/intel_th/msu.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Add a flexible array member to mark the end of struct nlmsghdr, and
split the memcpy() to avoid false positive memcpy() warning:
memcpy: detected field-spanning write (size 32) of single field (size 16)
Signed-off-by: Kees Cook <redacted>
---
include/uapi/linux/netlink.h | 1 +
net/netlink/af_netlink.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct hfa384x_tx_frame around members
frame_control, duration_id, addr1, addr2, addr3, and seq_ctrl, so they
can be referenced together. This will allow memcpy() and sizeof() to
more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of frame_control.
"pahole" shows no size nor member offset changes to struct
hfa384x_tx_frame. "objdump -d" shows no object code changes.
Signed-off-by: Kees Cook <redacted>
---
drivers/net/wireless/intersil/hostap/hostap_hw.c | 5 +++--
drivers/net/wireless/intersil/hostap/hostap_wlan.h | 14 ++++++++------
2 files changed, 11 insertions(+), 8 deletions(-)
@@ -115,12 +115,14 @@ struct hfa384x_tx_frame {__le16tx_control;/* HFA384X_TX_CTRL_ flags *//* 802.11 */-__le16frame_control;/* parts not used */-__le16duration_id;-u8addr1[ETH_ALEN];-u8addr2[ETH_ALEN];/* filled by firmware */-u8addr3[ETH_ALEN];-__le16seq_ctrl;/* filled by firmware */+struct_group(frame,+__le16frame_control;/* parts not used */+__le16duration_id;+u8addr1[ETH_ALEN];+u8addr2[ETH_ALEN];/* filled by firmware */+u8addr3[ETH_ALEN];+__le16seq_ctrl;/* filled by firmware */+);u8addr4[ETH_ALEN];__le16data_len;
A common idiom in kernel code is to wipe the contents of a structure
after a given member. This includes places where there is trailing
struct padding. These open-coded cases are usually difficult to read and
very sensitive to struct layout changes. Introduce a new helper,
memset_after() that takes the target struct instance, the byte to
write, and the member name after which the zeroing should start.
Signed-off-by: Kees Cook <redacted>
---
include/linux/string.h | 12 ++++++++++++
lib/test_memcpy.c | 12 ++++++++++++
2 files changed, 24 insertions(+)
Clang has never correctly compiled the FORTIFY_SOURCE defenses due to
a couple bugs:
Eliding inlines with matching __builtin_* names
https://bugs.llvm.org/show_bug.cgi?id=50322
Incorrect __builtin_constant_p() of some globals
https://bugs.llvm.org/show_bug.cgi?id=41459
In the process of making improvements to the FORTIFY_SOURCE defenses, the
first (silent) bug (coincidentally) becomes worked around, but exposes
the latter which breaks the build. As such, Clang must not be used with
CONFIG_FORTIFY_SOURCE until at least latter bug is fixed (in Clang 13),
and the fortify routines have been rearranged.
Update the Kconfig to reflect the reality of the current situation.
Signed-off-by: Kees Cook <redacted>
---
security/Kconfig | 3 +++
1 file changed, 3 insertions(+)
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add a struct_group() for the algs so that memset() can correctly reason
about the size.
Signed-off-by: Kees Cook <redacted>
---
drivers/block/drbd/drbd_main.c | 3 ++-
drivers/block/drbd/drbd_protocol.h | 6 ++++--
drivers/block/drbd/drbd_receiver.c | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
On Tue, Jul 27, 2021 at 02:18:58PM -0700, Nathan Chancellor wrote:
On 7/27/2021 1:58 PM, Kees Cook wrote:
quoted
Clang has never correctly compiled the FORTIFY_SOURCE defenses due to
a couple bugs:
Eliding inlines with matching __builtin_* names
https://bugs.llvm.org/show_bug.cgi?id=50322
Incorrect __builtin_constant_p() of some globals
https://bugs.llvm.org/show_bug.cgi?id=41459
In the process of making improvements to the FORTIFY_SOURCE defenses, the
first (silent) bug (coincidentally) becomes worked around, but exposes
the latter which breaks the build. As such, Clang must not be used with
CONFIG_FORTIFY_SOURCE until at least latter bug is fixed (in Clang 13),
and the fortify routines have been rearranged.
Update the Kconfig to reflect the reality of the current situation.
Signed-off-by: Kees Cook <redacted>
---
security/Kconfig | 3 +++
1 file changed, 3 insertions(+)
From: Larry Finger <hidden> Date: 2021-07-27 22:30:54
On 7/27/21 3:57 PM, Kees Cook wrote:
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() around members addr1, addr2, and addr3 in struct
rtllib_hdr_4addr, and members qui, qui_type, qui_subtype, version,
and ac_info in struct rtllib_qos_information_element, so they can be
referenced together. This will allow memcpy() and sizeof() to more easily
reason about sizes, improve readability, and avoid future warnings about
writing beyond the end of addr1 and qui.
"pahole" shows no size nor member offset changes to struct
rtllib_hdr_4addr nor struct rtllib_qos_information_element. "objdump -d"
shows no meaningful object code changes (i.e. only source line number
induced differences and optimizations).
Signed-off-by: Kees Cook<redacted>
Tested-by: Larry Finger <redacted>
Acked-by: Larry Finger <redacted>
Looks good.
Larry
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2021-07-27 22:43:44
On Tue, Jul 27, 2021 at 2:17 PM Kees Cook [off-list ref] 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
+
#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
#define __RENAME(x) __asm__(#x)
void fortify_panic(const char *name) __noreturn __cold;
void __read_overflow(void) __compiletime_error("detected read beyond size of object (1st parameter)");
void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
+void __read_overflow2_field(void) __compiletime_warning("detected read beyond size of field (2nd parameter); maybe use struct_group()?");
void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
+void __write_overflow_field(void) __compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?");
#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size) __RENAME(memchr);
@@ -182,22 +186,105 @@ __FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size) return __underlying_memset(p, c, size); }-__FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size)+/*+ * To make sure the compiler can enforce protection against buffer overflows,+ * memcpy(), memmove(), and memset() must not be used beyond individual+ * struct members. If you need to copy across multiple members, please use+ * struct_group() to create a named mirror of an anonymous struct union.+ * (e.g. see struct sk_buff.)+ *+ * Mitigation coverage+ * Bounds checking at:+ * +-------+-------+-------+-------++ * | Compile time | Run time |+ * memcpy() argument sizes: | write | read | write | read |+ * +-------+-------+-------+-------++ * memcpy(known, known, constant) | y | y | n/a | n/a |+ * memcpy(unknown, known, constant) | n | y | V | n/a |+ * memcpy(known, unknown, constant) | y | n | n/a | V |+ * memcpy(unknown, unknown, constant) | n | n | V | V |+ * memcpy(known, known, dynamic) | n | n | b | B |+ * memcpy(unknown, known, dynamic) | n | n | V | B |+ * memcpy(known, unknown, dynamic) | n | n | b | V |+ * memcpy(unknown, unknown, dynamic) | n | n | V | V |+ * +-------+-------+-------+-------++ *+ * y = deterministic compile-time bounds checking+ * n = cannot do deterministic compile-time bounds checking+ * n/a = no run-time bounds checking needed since compile-time deterministic+ * b = perform run-time bounds checking+ * B = can perform run-time bounds checking, but current unenforced+ * V = vulnerable to run-time overflow+ *+ */+__FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,+ const size_t p_size,+ const size_t q_size,+ const size_t p_size_field,+ const size_t q_size_field,+ const char *func) {- size_t p_size = __builtin_object_size(p, 0);- size_t q_size = __builtin_object_size(q, 0);- if (__builtin_constant_p(size)) {- if (p_size < size)+ /*+ * Length argument is a constant expression, so we+ * can perform compile-time bounds checking where+ * buffer sizes are known.+ */++ /* Error when size is larger than enclosing struct. */+ if (p_size > p_size_field && p_size < size) __write_overflow();- if (q_size < size)+ if (q_size > q_size_field && q_size < size) __read_overflow2();++ /* Warn when write size argument larger than dest field. */+ if (p_size_field < size)+ __write_overflow_field();+ /*+ * Warn for source field over-read when building with W=1+ * or when an over-write happened, so both can be fixed at+ * the same time.+ */+ if ((IS_ENABLED(KBUILD_EXTRA_WARN1) || p_size_field < size) &&+ q_size_field < size)+ __read_overflow2_field(); }- if (p_size < size || q_size < size)- fortify_panic(__func__);- return __underlying_memcpy(p, q, size);+ /*+ * At this point, length argument may not be a constant expression,+ * so run-time bounds checking can be done where buffer sizes are+ * known. (This is not an "else" because the above checks may only+ * be compile-time warnings, and we want to still warn for run-time+ * overflows.)+ */++ /*+ * Always stop accesses beyond the struct that contains the+ * field, when the buffer's remaining size is known.+ * (The -1 test is to optimize away checks where the buffer+ * lengths are unknown.)+ */+ if ((p_size != (size_t)(-1) && p_size < size) ||+ (q_size != (size_t)(-1) && q_size < size))+ fortify_panic(func); }+#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); \+})
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__`?
quoted hunk
+
+/*
+ * __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)
+
__FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size)
{
size_t p_size = __builtin_object_size(p, 0);
@@ -277,27 +364,27 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp) return __real_kmemdup(p, size, gfp); }-/* defined after fortified strlen and memcpy to reuse them */+/* Defined after fortified strlen to reuse it. */ __FORTIFY_INLINE char *strcpy(char *p, const char *q) { size_t p_size = __builtin_object_size(p, 1); size_t q_size = __builtin_object_size(q, 1); size_t size;+ /* If neither buffer size is known, immediately give up. */ if (p_size == (size_t)-1 && q_size == (size_t)-1) return __underlying_strcpy(p, q); size = strlen(q) + 1; /* test here to use the more stringent object size */ if (p_size < size) fortify_panic(__func__);- memcpy(p, q, size);+ __underlying_memcpy(p, q, size); return p; } /* Don't use these outside the FORITFY_SOURCE implementation */ #undef __underlying_memchr #undef __underlying_memcmp-#undef __underlying_memcpy #undef __underlying_memmove #undef __underlying_memset #undef __underlying_strcat
@@ -884,6 +884,12 @@ char *strreplace(char *s, char old, char new)EXPORT_SYMBOL(strreplace);#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?
From: "Gustavo A. R. Silva" <gustavoars@kernel.org> Date: 2021-07-28 00:53:22
On Tue, Jul 27, 2021 at 01:57:52PM -0700, Kees Cook wrote:
quoted hunk
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields. Wrap the target region
in a common named structure. This additionally fixes a theoretical
misalignment of the copy (since the size of "buf" changes between 64-bit
and 32-bit, but this is likely never built for 64-bit).
FWIW, I think this code is totally broken on 64-bit (which appears to
not be a "real" build configuration): it would either always fail (with
an uninitialized data->buf_size) or would cause corruption in userspace
due to the copy_to_user() in the call path against an uninitialized
data->buf value:
omap3isp_stat_request_statistics_time32(...)
struct omap3isp_stat_data data64;
...
omap3isp_stat_request_statistics(stat, &data64);
int omap3isp_stat_request_statistics(struct ispstat *stat,
struct omap3isp_stat_data *data)
...
buf = isp_stat_buf_get(stat, data);
static struct ispstat_buffer *isp_stat_buf_get(struct ispstat *stat,
struct omap3isp_stat_data *data)
...
if (buf->buf_size > data->buf_size) {
...
return ERR_PTR(-EINVAL);
}
...
rval = copy_to_user(data->buf,
buf->virt_addr,
buf->buf_size);
Regardless, additionally initialize data64 to be zero-filled to avoid
undefined behavior.
Fixes: 378e3f81cb56 ("media: omap3isp: support 64-bit version of omap3isp_stat_data")
Signed-off-by: Kees Cook <redacted>
---
drivers/media/platform/omap3isp/ispstat.c | 5 +--
include/uapi/linux/omap3isp.h | 44 +++++++++++++++++------
2 files changed, 36 insertions(+), 13 deletions(-)
From: Michael Chan <michael.chan@broadcom.com> Date: 2021-07-28 01:03:38
On Tue, Jul 27, 2021 at 2:01 PM Kees Cook [off-list ref] wrote:
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() around members queue_id, min_bw, max_bw, tsa, pri_lvl,
and bw_weight so they can be referenced together. This will allow memcpy()
and sizeof() to more easily reason about sizes, improve readability,
and avoid future warnings about writing beyond the end of queue_id.
"pahole" shows no size nor member offset changes to struct bnxt_cos2bw_cfg.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Signed-off-by: Kees Cook <redacted>
Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
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.)
[...]
+#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__`?
[...]
* @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.