[PATCH 00/64] Introduce strict memcpy() bounds checking

[Kees Cook]

Hi,

This patch series (based on next-20210726) implements stricter (no struct
member overflows) bounds checking for memcpy(), memmove(), and memset()
under CONFIG_FORTIFY_SOURCE. To quote a later patch in the series:

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.

As this series introduces various helpers and performs several phases of
treewide cleanups, I'm expecting to carry this series in my tree, so I'd
love to get some Reviews and Acks. Given the size, I've mostly aimed this
series at various mailing lists, otherwise the CC size got really big. :)

Specifically, this series is logically split into several steps:

Clean up remaining simple compile-time memcpy() warnings:
media: omap3isp: Extract struct group for memcpy() region
mac80211: Use flex-array for radiotap header bitmap
rpmsg: glink: Replace strncpy() with strscpy_pad()

Introduce struct_group() and apply it treewide to avoid compile-time
memcpy() warnings:
stddef: Introduce struct_group() helper macro
skbuff: Switch structure bounds to struct_group()
bnxt_en: Use struct_group_attr() for memcpy() region
staging: rtl8192e: Use struct_group() for memcpy() region
staging: rtl8192u: Use struct_group() for memcpy() region
staging: rtl8723bs: Avoid field-overflowing memcpy()
lib80211: Use struct_group() for memcpy() region
net/mlx5e: Avoid field-overflowing memcpy()
mwl8k: Use struct_group() for memcpy() region
libertas: Use struct_group() for memcpy() region
libertas_tf: Use struct_group() for memcpy() region
ipw2x00: Use struct_group() for memcpy() region
thermal: intel: int340x_thermal: Use struct_group() for memcpy() region
iommu/amd: Use struct_group() for memcpy() region
cxgb3: Use struct_group() for memcpy() region
ip: Use struct_group() for memcpy() regions
intersil: Use struct_group() for memcpy() region
cxgb4: Use struct_group() for memcpy() region
bnx2x: Use struct_group() for memcpy() region
drm/amd/pm: Use struct_group() for memcpy() region
staging: wlan-ng: Use struct_group() for memcpy() region
drm/mga/mga_ioc32: Use struct_group() for memcpy() region
net/mlx5e: Use struct_group() for memcpy() region
HID: cp2112: Use struct_group() for memcpy() region

Prepare fortify for additional hardening:
compiler_types.h: Remove __compiletime_object_size()
lib/string: Move helper functions out of string.c
fortify: Move remaining fortify helpers into fortify-string.h
fortify: Explicitly disable Clang support

Add compile-time and run-time tests:
fortify: Add compile-time FORTIFY_SOURCE tests
lib: Introduce CONFIG_TEST_MEMCPY

Enable new compile-time memcpy() and memmove() bounds checking:
fortify: Detect struct member overflows in memcpy() at compile-time
fortify: Detect struct member overflows in memmove() at compile-time

Clean up remaining simple compile-time memset() warnings:
scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp

Introduce memset_after() helper and apply it (and struct_group())
treewide to avoid compile-time memset() warnings:
string.h: Introduce memset_after() for wiping trailing members/padding
xfrm: Use memset_after() to clear padding
mac80211: Use memset_after() to clear tx status
net: 802: Use memset_after() to clear struct fields
net: dccp: Use memset_after() for TP zeroing
net: qede: Use memset_after() for counters
ath11k: Use memset_after() for clearing queue descriptors
iw_cxgb4: Use memset_after() for cpl_t5_pass_accept_rpl
intel_th: msu: Use memset_after() for clearing hw header
IB/mthca: Use memset_after() for clearing mpt_entry
btrfs: Use memset_after() to clear end of struct
drbd: Use struct_group() to zero algs
cm4000_cs: Use struct_group() to zero struct cm4000_dev region
KVM: x86: Use struct_group() to zero decode cache
tracing: Use struct_group() to zero struct trace_iterator
dm integrity: Use struct_group() to zero struct journal_sector
HID: roccat: Use struct_group() to zero kone_mouse_event
ipv6: Use struct_group() to zero rt6_info
RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
ethtool: stats: Use struct_group() to clear all stats at once
netfilter: conntrack: Use struct_group() to zero struct nf_conn
powerpc: Split memset() to avoid multi-field overflow

Enable new compile-time memset() bounds checking:
fortify: Detect struct member overflows in memset() at compile-time

Enable Clang support and global array-bounds checking:
fortify: Work around Clang inlining bugs
Makefile: Enable -Warray-bounds

Avoid run-time memcpy() bounds check warnings:
netlink: Avoid false-positive memcpy() warning
iwlwifi: dbg_ini: Split memcpy() to avoid multi-field write

Enable run-time memcpy() bounds checking:
fortify: Add run-time WARN for cross-field memcpy()

A future series will clean up for and add run-time memset() bounds
checking.

Thanks!

-Kees


Makefile | 1 -
arch/s390/lib/string.c | 3 +
arch/x86/boot/compressed/misc.c | 3 +-
arch/x86/kvm/emulate.c | 3 +-
arch/x86/kvm/kvm_emulate.h | 19 +-
arch/x86/lib/memcpy_32.c | 1 +
arch/x86/lib/string_32.c | 1 +
drivers/block/drbd/drbd_main.c | 3 +-
drivers/block/drbd/drbd_protocol.h | 6 +-
drivers/block/drbd/drbd_receiver.c | 3 +-
drivers/char/pcmcia/cm4000_cs.c | 9 +-
drivers/gpu/drm/amd/include/atomfirmware.h | 9 +-
.../drm/amd/pm/inc/smu11_driver_if_arcturus.h | 3 +-
.../drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 +-
.../amd/pm/inc/smu13_driver_if_aldebaran.h | 3 +-
.../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 +-
.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 12 +-
.../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 +-
drivers/gpu/drm/mga/mga_ioc32.c | 30 +-
drivers/hid/hid-cp2112.c | 14 +-
drivers/hid/hid-roccat-kone.c | 2 +-
drivers/hid/hid-roccat-kone.h | 12 +-
drivers/hwtracing/intel_th/msu.c | 4 +-
drivers/infiniband/hw/cxgb4/cm.c | 5 +-
drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +-
drivers/infiniband/hw/mthca/mthca_mr.c | 3 +-
drivers/iommu/amd/init.c | 9 +-
drivers/macintosh/smu.c | 3 +-
drivers/md/dm-integrity.c | 9 +-
drivers/media/platform/omap3isp/ispstat.c | 5 +-
.../net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 7 +-
.../net/ethernet/broadcom/bnx2x/bnx2x_stats.h | 14 +-
drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c | 4 +-
drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h | 14 +-
drivers/net/ethernet/chelsio/cxgb3/sge.c | 9 +-
drivers/net/ethernet/chelsio/cxgb4/sge.c | 8 +-
drivers/net/ethernet/chelsio/cxgb4/t4_msg.h | 2 +-
drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h | 10 +-
drivers/net/ethernet/chelsio/cxgb4vf/sge.c | 7 +-
drivers/net/ethernet/mellanox/mlx5/core/en.h | 4 +-
.../net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 +-
.../net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +-
drivers/net/ethernet/qlogic/qede/qede_main.c | 2 +-
drivers/net/wireguard/queueing.h | 4 +-
drivers/net/wireless/ath/ath11k/hal_rx.c | 13 +-
drivers/net/wireless/ath/carl9170/tx.c | 4 +-
drivers/net/wireless/intel/ipw2x00/libipw.h | 12 +-
.../net/wireless/intel/ipw2x00/libipw_rx.c | 8 +-
drivers/net/wireless/intel/iwlwifi/fw/file.h | 2 +-
.../net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 3 +-
.../net/wireless/intersil/hostap/hostap_hw.c | 5 +-
.../wireless/intersil/hostap/hostap_wlan.h | 14 +-
drivers/net/wireless/intersil/p54/txrx.c | 4 +-
drivers/net/wireless/marvell/libertas/host.h | 10 +-
drivers/net/wireless/marvell/libertas/tx.c | 5 +-
.../marvell/libertas_tf/libertas_tf.h | 10 +-
.../net/wireless/marvell/libertas_tf/main.c | 3 +-
drivers/net/wireless/marvell/mwl8k.c | 10 +-
drivers/rpmsg/qcom_glink_native.c | 2 +-
drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
drivers/staging/rtl8192e/rtllib.h | 20 +-
drivers/staging/rtl8192e/rtllib_crypt_ccmp.c | 3 +-
drivers/staging/rtl8192e/rtllib_rx.c | 8 +-
.../staging/rtl8192u/ieee80211/ieee80211.h | 24 +-
.../rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 3 +-
.../staging/rtl8192u/ieee80211/ieee80211_rx.c | 8 +-
drivers/staging/rtl8723bs/core/rtw_mlme.c | 2 +-
drivers/staging/rtl8723bs/core/rtw_security.c | 5 +-
drivers/staging/rtl8723bs/core/rtw_xmit.c | 5 +-
drivers/staging/wlan-ng/hfa384x.h | 16 +-
drivers/staging/wlan-ng/hfa384x_usb.c | 4 +-
.../intel/int340x_thermal/acpi_thermal_rel.c | 5 +-
.../intel/int340x_thermal/acpi_thermal_rel.h | 48 +--
fs/btrfs/root-tree.c | 5 +-
include/linux/compiler-gcc.h | 2 -
include/linux/compiler_types.h | 4 -
include/linux/fortify-string.h | 234 +++++++++++---
include/linux/ieee80211.h | 8 +-
include/linux/if_vlan.h | 6 +-
include/linux/skbuff.h | 9 +-
include/linux/stddef.h | 34 ++
include/linux/string.h | 26 +-
include/linux/thread_info.h | 2 +-
include/linux/trace_events.h | 26 +-
include/net/flow.h | 6 +-
include/net/ieee80211_radiotap.h | 24 +-
include/net/ip6_fib.h | 30 +-
include/net/mac80211.h | 4 +-
include/net/netfilter/nf_conntrack.h | 20 +-
include/uapi/drm/mga_drm.h | 37 ++-
include/uapi/linux/if_ether.h | 12 +-
include/uapi/linux/ip.h | 12 +-
include/uapi/linux/ipv6.h | 12 +-
include/uapi/linux/netlink.h | 1 +
include/uapi/linux/omap3isp.h | 44 ++-
kernel/trace/trace.c | 4 +-
lib/.gitignore | 2 +
lib/Kconfig.debug | 3 +
lib/Makefile | 32 ++
lib/string.c | 210 +------------
lib/string_helpers.c | 201 ++++++++++++
lib/test_fortify/read_overflow-memchr.c | 5 +
lib/test_fortify/read_overflow-memchr_inv.c | 5 +
lib/test_fortify/read_overflow-memcmp.c | 5 +
lib/test_fortify/read_overflow-memscan.c | 5 +
lib/test_fortify/read_overflow2-memcmp.c | 5 +
lib/test_fortify/read_overflow2-memcpy.c | 5 +
lib/test_fortify/read_overflow2-memmove.c | 5 +
.../read_overflow2_field-memcpy.c | 5 +
.../read_overflow2_field-memmove.c | 5 +
lib/test_fortify/test_fortify.h | 31 ++
lib/test_fortify/write_overflow-memcpy.c | 5 +
lib/test_fortify/write_overflow-memmove.c | 5 +
lib/test_fortify/write_overflow-memset.c | 5 +
lib/test_fortify/write_overflow-strlcpy.c | 5 +
lib/test_fortify/write_overflow-strncpy.c | 5 +
lib/test_fortify/write_overflow-strscpy.c | 5 +
.../write_overflow_field-memcpy.c | 5 +
.../write_overflow_field-memmove.c | 5 +
.../write_overflow_field-memset.c | 5 +
lib/test_memcpy.c | 297 ++++++++++++++++++
net/802/hippi.c | 2 +-
net/core/flow_dissector.c | 10 +-
net/core/skbuff.c | 14 +-
net/dccp/trace.h | 4 +-
net/ethtool/stats.c | 15 +-
net/ipv4/ip_output.c | 6 +-
net/ipv6/route.c | 4 +-
net/mac80211/rx.c | 2 +-
net/netfilter/nf_conntrack_core.c | 4 +-
net/netlink/af_netlink.c | 4 +-
net/wireless/lib80211_crypt_ccmp.c | 3 +-
net/wireless/radiotap.c | 5 +-
net/xfrm/xfrm_policy.c | 4 +-
net/xfrm/xfrm_user.c | 2 +-
scripts/test_fortify.sh | 64 ++++
security/Kconfig | 3 +
137 files changed, 1484 insertions(+), 633 deletions(-)
create mode 100644 lib/test_fortify/read_overflow-memchr.c
create mode 100644 lib/test_fortify/read_overflow-memchr_inv.c
create mode 100644 lib/test_fortify/read_overflow-memcmp.c
create mode 100644 lib/test_fortify/read_overflow-memscan.c
create mode 100644 lib/test_fortify/read_overflow2-memcmp.c
create mode 100644 lib/test_fortify/read_overflow2-memcpy.c
create mode 100644 lib/test_fortify/read_overflow2-memmove.c
create mode 100644 lib/test_fortify/read_overflow2_field-memcpy.c
create mode 100644 lib/test_fortify/read_overflow2_field-memmove.c
create mode 100644 lib/test_fortify/test_fortify.h
create mode 100644 lib/test_fortify/write_overflow-memcpy.c
create mode 100644 lib/test_fortify/write_overflow-memmove.c
create mode 100644 lib/test_fortify/write_overflow-memset.c
create mode 100644 lib/test_fortify/write_overflow-strlcpy.c
create mode 100644 lib/test_fortify/write_overflow-strncpy.c
create mode 100644 lib/test_fortify/write_overflow-strscpy.c
create mode 100644 lib/test_fortify/write_overflow_field-memcpy.c
create mode 100644 lib/test_fortify/write_overflow_field-memmove.c
create mode 100644 lib/test_fortify/write_overflow_field-memset.c
create mode 100644 lib/test_memcpy.c
create mode 100644 scripts/test_fortify.sh

--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---
drivers/staging/rtl8723bs/core/rtw_mlme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
index a0c19253095d..fbd6e3d16323 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
@@ -2390,7 +2390,7 @@ unsigned int rtw_restructure_ht_ie(struct adapter *padapter, u8 *in_ie, u8 *out_
}

/* fill default supported_mcs_set */
- memcpy(ht_capie.mcs.rx_mask, pmlmeext->default_supported_mcs_set, 16);
+ memcpy(&ht_capie.mcs, pmlmeext->default_supported_mcs_set, 16);

/* update default supported_mcs_set */
rtw_hal_get_hwreg(padapter, HW_VAR_RF_TYPE, (u8 *)(&rf_type));
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/net/wireguard/queueing.h | 4 +---
include/linux/skbuff.h | 9 ++++-----
net/core/skbuff.c | 14 +++++---------
3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h
index 4ef2944a68bc..52da5e963003 100644
--- a/drivers/net/wireguard/queueing.h
+++ b/drivers/net/wireguard/queueing.h
@@ -79,9 +79,7 @@ static inline void wg_reset_packet(struct sk_buff *skb, bool encapsulating)
u8 sw_hash = skb->sw_hash;
u32 hash = skb->hash;
skb_scrub_packet(skb, true);
- memset(&skb->headers_start, 0,
- offsetof(struct sk_buff, headers_end) -
- offsetof(struct sk_buff, headers_start));
+ memset(&skb->headers, 0, sizeof(skb->headers));
if (encapsulating) {
skb->l4_hash = l4_hash;
skb->sw_hash = sw_hash;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f19190820e63..b4032e9b130e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -800,11 +800,10 @@ struct sk_buff {
__u8 active_extensions;
#endif

- /* fields enclosed in headers_start/headers_end are copied
+ /* Fields enclosed in headers group are copied
* using a single memcpy() in __copy_skb_header()
*/
- /* private: */
- __u32 headers_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 {
u64 kcov_handle;
#endif

- /* private: */
- __u32 headers_end[0];
+ ); /* end headers group */
+
/* public: */

/* These elements must be at the end, see alloc_skb() for details. */
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index fc7942c0dddc..5f29c65507e0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -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(struct sk_buff, field) < \
- offsetof(struct sk_buff, headers_start)); \
- BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
- offsetof(struct sk_buff, headers_end)); \
+ BUILD_BUG_ON(offsetof(struct sk_buff, field) != \
+ offsetof(struct sk_buff, headers.field)); \

static void __copy_skb_header(struct sk_buff *new, const struct sk_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.
* It is not yet because we do not want to have a 16 bit hole
*/
new->queue_mapping = old->queue_mapping;

- memcpy(&new->headers_start, &old->headers_start,
- offsetof(struct sk_buff, headers_end) -
- offsetof(struct sk_buff, headers_start));
+ memcpy(&new->headers, &old->headers, sizeof(new->headers));
CHECK_SKB_FIELD(protocol);
CHECK_SKB_FIELD(csum);
CHECK_SKB_FIELD(hash);
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
index 8a68df4d9e59..95c636f89329 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
@@ -148,10 +148,10 @@ static int bnxt_hwrm_queue_cos2bw_qcfg(struct bnxt *bp, struct ieee_ets *ets)
}

data = &resp->queue_id0 + offsetof(struct bnxt_cos2bw_cfg, queue_id);
- for (i = 0; i < bp->max_tc; i++, data += sizeof(cos2bw) - 4) {
+ for (i = 0; i < bp->max_tc; i++, data += sizeof(cos2bw.cfg)) {
int tc;

- memcpy(&cos2bw.queue_id, data, sizeof(cos2bw) - 4);
+ memcpy(&cos2bw.cfg, data, sizeof(cos2bw.cfg));
if (i == 0)
cos2bw.queue_id = resp->queue_id0;

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h
index 6eed231de565..716742522161 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h
@@ -23,13 +23,15 @@ struct bnxt_dcb {

struct bnxt_cos2bw_cfg {
u8 pad[3];
- u8 queue_id;
- __le32 min_bw;
- __le32 max_bw;
+ struct_group_attr(cfg, __packed,
+ u8 queue_id;
+ __le32 min_bw;
+ __le32 max_bw;
#define BW_VALUE_UNIT_PERCENT1_100 (0x1UL << 29)
- u8 tsa;
- u8 pri_lvl;
- u8 bw_weight;
+ u8 tsa;
+ u8 pri_lvl;
+ u8 bw_weight;
+ );
u8 unused;
};

--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c
index a99f439328f1..be7cf42855a1 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -1421,8 +1421,9 @@ u32 rtw_BIP_verify(struct adapter *padapter, u8 *precvframe)
ClearRetry(BIP_AAD);
ClearPwrMgt(BIP_AAD);
ClearMData(BIP_AAD);
- /* conscruct AAD, copy address 1 to address 3 */
- memcpy(BIP_AAD+2, pwlanhdr->addr1, 18);
+ /* conscruct AAD, copy address 1 through address 3 */
+ BUILD_BUG_ON(sizeof(pwlanhdr->addrs) != 3 * ETH_ALEN);
+ memcpy(BIP_AAD + 2, &pwlanhdr->addrs, 3 * ETH_ALEN);

if (omac1_aes_128(padapter->securitypriv.dot11wBIPKey[padapter->securitypriv.dot11wBIPKeyid].skey
, BIP_AAD, ori_len, mic))
diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 79e4d7df1ef5..cb47db784130 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -1198,8 +1198,9 @@ s32 rtw_mgmt_xmitframe_coalesce(struct adapter *padapter, struct sk_buff *pkt, s
ClearRetry(BIP_AAD);
ClearPwrMgt(BIP_AAD);
ClearMData(BIP_AAD);
- /* conscruct AAD, copy address 1 to address 3 */
- memcpy(BIP_AAD+2, pwlanhdr->addr1, 18);
+ /* conscruct AAD, copy address 1 through address 3 */
+ BUILD_BUG_ON(sizeof(pwlanhdr->addrs) != 3 * ETH_ALEN);
+ memcpy(BIP_AAD + 2, &pwlanhdr->addrs, 3 * ETH_ALEN);
/* copy management fram body */
memcpy(BIP_AAD+BIP_AAD_SIZE, MGMT_body, frame_body_len);
/* calculate mic */
diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index a6730072d13a..d7932b520aaf 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -297,9 +297,11 @@ static inline u16 ieee80211_sn_sub(u16 sn1, u16 sn2)
struct ieee80211_hdr {
__le16 frame_control;
__le16 duration_id;
- u8 addr1[ETH_ALEN];
- u8 addr2[ETH_ALEN];
- u8 addr3[ETH_ALEN];
+ struct_group(addrs,
+ u8 addr1[ETH_ALEN];
+ u8 addr2[ETH_ALEN];
+ u8 addr3[ETH_ALEN];
+ );
__le16 seq_ctrl;
u8 addr4[ETH_ALEN];
} __packed __aligned(2);
diff --git a/net/wireless/lib80211_crypt_ccmp.c b/net/wireless/lib80211_crypt_ccmp.c
index 6a5f08f7491e..21d7c39bb394 100644
--- a/net/wireless/lib80211_crypt_ccmp.c
+++ b/net/wireless/lib80211_crypt_ccmp.c
@@ -136,7 +136,8 @@ static int ccmp_init_iv_and_aad(const struct ieee80211_hdr *hdr,
pos = (u8 *) hdr;
aad[0] = pos[0] & 0x8f;
aad[1] = pos[1] & 0xc7;
- memcpy(aad + 2, hdr->addr1, 3 * ETH_ALEN);
+ BUILD_BUG_ON(sizeof(hdr->addrs) != 3 * ETH_ALEN);
+ memcpy(aad + 2, &hdr->addrs, ETH_ALEN);
pos = (u8 *) & hdr->seq_ctrl;
aad[20] = pos[0] & 0x0f;
aad[21] = 0; /* all bits masked */
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
index c6f8b772335c..547579070a82 100644
--- a/drivers/staging/rtl8192e/rtllib.h
+++ b/drivers/staging/rtl8192e/rtllib.h
@@ -759,9 +759,11 @@ struct rtllib_hdr_3addr {
struct rtllib_hdr_4addr {
__le16 frame_ctl;

__le16 duration_id;
- u8 addr1[ETH_ALEN];
- u8 addr2[ETH_ALEN];
- u8 addr3[ETH_ALEN];
+ struct_group(addrs,
+ u8 addr1[ETH_ALEN];
+ u8 addr2[ETH_ALEN];
+ u8 addr3[ETH_ALEN];
+ );

__le16 seq_ctl;
u8 addr4[ETH_ALEN];
u8 payload[];
@@ -921,11 +923,13 @@ union frameqos {
struct rtllib_qos_information_element {
u8 elementID;
u8 length;
- u8 qui[QOS_OUI_LEN];
- u8 qui_type;
- u8 qui_subtype;
- u8 version;
- u8 ac_info;
+ struct_group(data,
+ u8 qui[QOS_OUI_LEN];
+ u8 qui_type;
+ u8 qui_subtype;
+ u8 version;
+ u8 ac_info;
+ );
} __packed;

struct rtllib_qos_ac_parameter {
diff --git a/drivers/staging/rtl8192e/rtllib_crypt_ccmp.c b/drivers/staging/rtl8192e/rtllib_crypt_ccmp.c
index b60e2a109ce4..66b3a13fced7 100644
--- a/drivers/staging/rtl8192e/rtllib_crypt_ccmp.c
+++ b/drivers/staging/rtl8192e/rtllib_crypt_ccmp.c
@@ -133,7 +133,8 @@ static int ccmp_init_iv_and_aad(struct rtllib_hdr_4addr *hdr,

pos = (u8 *) hdr;
aad[0] = pos[0] & 0x8f;
aad[1] = pos[1] & 0xc7;
- memcpy(aad + 2, hdr->addr1, 3 * ETH_ALEN);

+ BUILD_BUG_ON(sizeof(hdr->addrs) != (3 * ETH_ALEN));
+ memcpy(aad + 2, &hdr->addrs, 3 * ETH_ALEN);
pos = (u8 *) &hdr->seq_ctl;

aad[20] = pos[0] & 0x0f;
aad[21] = 0; /* all bits masked */

diff --git a/drivers/staging/rtl8192e/rtllib_rx.c b/drivers/staging/rtl8192e/rtllib_rx.c
index c2209c033838..9c4b686d2756 100644
--- a/drivers/staging/rtl8192e/rtllib_rx.c
+++ b/drivers/staging/rtl8192e/rtllib_rx.c
@@ -1576,13 +1576,13 @@ static int rtllib_read_qos_param_element(struct rtllib_qos_parameter_info
*info_element)
{
int ret = 0;
- u16 size = sizeof(struct rtllib_qos_parameter_info) - 2;
+ u16 size = sizeof(element_param->info_element.data);

if ((info_element == NULL) || (element_param == NULL))
return -1;

if (info_element->id == QOS_ELEMENT_ID && info_element->len == size) {
- memcpy(element_param->info_element.qui, info_element->data,
+ memcpy(&element_param->info_element.data, info_element->data,
info_element->len);
element_param->info_element.elementID = info_element->id;
element_param->info_element.length = info_element->len;
@@ -1601,7 +1601,7 @@ static int rtllib_read_qos_info_element(struct rtllib_qos_information_element
*info_element)
{
int ret = 0;
- u16 size = sizeof(struct rtllib_qos_information_element) - 2;
+ u16 size = sizeof(element_info->data);

if (element_info == NULL)
return -1;
@@ -1610,7 +1610,7 @@ static int rtllib_read_qos_info_element(struct rtllib_qos_information_element

if ((info_element->id == QOS_ELEMENT_ID) &&
(info_element->len == size)) {
- memcpy(element_info->qui, info_element->data,
+ memcpy(&element_info->data, info_element->data,
info_element->len);
element_info->elementID = info_element->id;
element_info->length = info_element->len;
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211.h b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
index 7903c777a417..6ae8d7504558 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
@@ -303,7 +303,7 @@ struct ieee_param {
struct {
u32 len;
u8 reserved[32];
- u8 data[0];
+ u8 data[];
} wpa_ie;
struct{
int command;
@@ -316,7 +316,7 @@ struct ieee_param {
u8 idx;
u8 seq[8]; /* sequence counter (set: RX, get: TX) */
u16 key_len;
- u8 key[0];
+ u8 key[];
} crypt;
} u;
};
@@ -917,9 +917,11 @@ struct rtl_80211_hdr_3addr {
struct rtl_80211_hdr_4addr {

__le16 frame_ctl;
__le16 duration_id;
- u8 addr1[ETH_ALEN];
- u8 addr2[ETH_ALEN];
- u8 addr3[ETH_ALEN];
+ struct_group(addrs,
+ u8 addr1[ETH_ALEN];
+ u8 addr2[ETH_ALEN];
+ u8 addr3[ETH_ALEN];
+ );
__le16 seq_ctl;
u8 addr4[ETH_ALEN];
u8 payload[];

@@ -1100,11 +1102,13 @@ typedef union _frameqos {
struct ieee80211_qos_information_element {

u8 elementID;
u8 length;
- u8 qui[QOS_OUI_LEN];
- u8 qui_type;
- u8 qui_subtype;
- u8 version;
- u8 ac_info;
+ struct_group(data,
+ u8 qui[QOS_OUI_LEN];
+ u8 qui_type;
+ u8 qui_subtype;
+ u8 version;
+ u8 ac_info;
+ );
} __packed;

struct ieee80211_qos_ac_parameter {
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c
index c241cf484023..61f9a189c90c 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c
@@ -141,7 +141,8 @@ static int ccmp_init_iv_and_aad(struct rtl_80211_hdr_4addr *hdr,
pos = (u8 *)hdr;

aad[0] = pos[0] & 0x8f;
aad[1] = pos[1] & 0xc7;
- memcpy(aad + 2, hdr->addr1, 3 * ETH_ALEN);

+ BUILD_BUG_ON(sizeof(hdr->addrs) != 3 * ETH_ALEN);

+ memcpy(aad + 2, &hdr->addrs, 3 * ETH_ALEN);

pos = (u8 *)&hdr->seq_ctl;

aad[20] = pos[0] & 0x0f;
aad[21] = 0; /* all bits masked */

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
index b0e01ee65f7f..d5557ee0a3b7 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
@@ -1332,13 +1332,13 @@ static int ieee80211_read_qos_param_element(struct ieee80211_qos_parameter_info

*info_element)
{
int ret = 0;

- u16 size = sizeof(struct ieee80211_qos_parameter_info) - 2;

+ u16 size = sizeof(element_param->info_element.data);

if (!info_element || !element_param)

return -1;

if (info_element->id == QOS_ELEMENT_ID && info_element->len == size) {
- memcpy(element_param->info_element.qui, info_element->data,
+ memcpy(&element_param->info_element.data, info_element->data,
info_element->len);
element_param->info_element.elementID = info_element->id;
element_param->info_element.length = info_element->len;

@@ -1358,7 +1358,7 @@ static int ieee80211_read_qos_info_element(
struct ieee80211_info_element *info_element)
{
int ret = 0;
- u16 size = sizeof(struct ieee80211_qos_information_element) - 2;

+ u16 size = sizeof(element_info->data);

if (!element_info)
return -1;
@@ -1366,7 +1366,7 @@ static int ieee80211_read_qos_info_element(
return -1;

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
index 0b193edb73b8..2bb133ae61c3 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
@@ -849,7 +849,8 @@ static int bnx2x_hw_stats_update(struct bnx2x *bp)

memcpy(old, new, sizeof(struct nig_stats));

- memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
+ BUILD_BUG_ON(sizeof(estats->shared) != sizeof(pstats->mac_stx[1]));
+ memcpy(&(estats->shared), &(pstats->mac_stx[1]),
sizeof(struct mac_stx));
estats->brb_drop_hi = pstats->brb_drop_hi;
estats->brb_drop_lo = pstats->brb_drop_lo;
@@ -1634,9 +1635,9 @@ void bnx2x_stats_init(struct bnx2x *bp)
REG_RD(bp, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
if (!CHIP_IS_E3(bp)) {
REG_RD_DMAE(bp, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
- &(bp->port.old_nig_stats.egress_mac_pkt0_lo), 2);
+ &(bp->port.old_nig_stats.egress_mac_pkt0), 2);
REG_RD_DMAE(bp, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
- &(bp->port.old_nig_stats.egress_mac_pkt1_lo), 2);
+ &(bp->port.old_nig_stats.egress_mac_pkt1), 2);
}

/* Prepare statistics ramrod data */
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h
index d55e63692cf3..ae93c078707b 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h
@@ -36,10 +36,14 @@ struct nig_stats {
u32 pbf_octets;
u32 pbf_packet;
u32 safc_inp;
- u32 egress_mac_pkt0_lo;
- u32 egress_mac_pkt0_hi;
- u32 egress_mac_pkt1_lo;
- u32 egress_mac_pkt1_hi;
+ struct_group(egress_mac_pkt0,
+ u32 egress_mac_pkt0_lo;
+ u32 egress_mac_pkt0_hi;
+ );
+ struct_group(egress_mac_pkt1,
+ u32 egress_mac_pkt1_lo;
+ u32 egress_mac_pkt1_hi;
+ );
};

enum bnx2x_stats_event {
@@ -83,6 +87,7 @@ struct bnx2x_eth_stats {
u32 no_buff_discard_hi;
u32 no_buff_discard_lo;

+ struct_group(shared,
u32 rx_stat_ifhcinbadoctets_hi;
u32 rx_stat_ifhcinbadoctets_lo;
u32 tx_stat_ifhcoutbadoctets_hi;
@@ -159,6 +164,7 @@ struct bnx2x_eth_stats {
u32 tx_stat_dot3statsinternalmactransmiterrors_lo;
u32 tx_stat_bmac_ufl_hi;
u32 tx_stat_bmac_ufl_lo;
+ );

u32 pause_frames_received_hi;
u32 pause_frames_received_lo;
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/net/wireless/marvell/libertas/host.h | 10 ++++++----
drivers/net/wireless/marvell/libertas/tx.c | 5 +++--
2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/host.h b/drivers/net/wireless/marvell/libertas/host.h
index dfa22468b14a..af96bdba3b2b 100644
--- a/drivers/net/wireless/marvell/libertas/host.h
+++ b/drivers/net/wireless/marvell/libertas/host.h
@@ -308,10 +308,12 @@ struct txpd {
__le32 tx_packet_location;
/* Tx packet length */
__le16 tx_packet_length;
- /* First 2 byte of destination MAC address */
- u8 tx_dest_addr_high[2];
- /* Last 4 byte of destination MAC address */
- u8 tx_dest_addr_low[4];
+ struct_group(tx_dest_addr,
+ /* First 2 byte of destination MAC address */
+ u8 tx_dest_addr_high[2];
+ /* Last 4 byte of destination MAC address */
+ u8 tx_dest_addr_low[4];
+ );
/* Pkt Priority */
u8 priority;
/* Pkt Trasnit Power control */
diff --git a/drivers/net/wireless/marvell/libertas/tx.c b/drivers/net/wireless/marvell/libertas/tx.c
index aeb481740df6..27304a98787d 100644
--- a/drivers/net/wireless/marvell/libertas/tx.c
+++ b/drivers/net/wireless/marvell/libertas/tx.c
@@ -113,6 +113,7 @@ netdev_tx_t lbs_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
p802x_hdr = skb->data;
pkt_len = skb->len;

+ BUILD_BUG_ON(sizeof(txpd->tx_dest_addr) != ETH_ALEN);
if (priv->wdev->iftype == NL80211_IFTYPE_MONITOR) {
struct tx_radiotap_hdr *rtap_hdr = (void *)skb->data;

@@ -124,10 +125,10 @@ netdev_tx_t lbs_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
pkt_len -= sizeof(*rtap_hdr);

/* copy destination address from 802.11 header */
- memcpy(txpd->tx_dest_addr_high, p802x_hdr + 4, ETH_ALEN);
+ memcpy(&txpd->tx_dest_addr, p802x_hdr + 4, ETH_ALEN);
} else {
/* copy destination address from 802.3 header */
- memcpy(txpd->tx_dest_addr_high, p802x_hdr, ETH_ALEN);
+ memcpy(&txpd->tx_dest_addr, p802x_hdr, ETH_ALEN);
}

txpd->tx_packet_length = cpu_to_le16(pkt_len);
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
index 5d726545d987..b2af2ddb6bc4 100644
--- a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
+++ b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h
@@ -268,10 +268,12 @@ struct txpd {

__le32 tx_packet_location;
/* Tx packet length */
__le16 tx_packet_length;
- /* First 2 byte of destination MAC address */
- u8 tx_dest_addr_high[2];
- /* Last 4 byte of destination MAC address */
- u8 tx_dest_addr_low[4];
+ struct_group(tx_dest_addr,
+ /* First 2 byte of destination MAC address */
+ u8 tx_dest_addr_high[2];
+ /* Last 4 byte of destination MAC address */
+ u8 tx_dest_addr_low[4];
+ );
/* Pkt Priority */
u8 priority;
/* Pkt Trasnit Power control */

diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c b/drivers/net/wireless/marvell/libertas_tf/main.c
index 71492211904b..02a1e1f547d8 100644
--- a/drivers/net/wireless/marvell/libertas_tf/main.c
+++ b/drivers/net/wireless/marvell/libertas_tf/main.c
@@ -232,7 +232,8 @@ static void lbtf_tx_work(struct work_struct *work)
ieee80211_get_tx_rate(priv->hw, info)->hw_value);

/* copy destination address from 802.11 header */

- memcpy(txpd->tx_dest_addr_high, skb->data + sizeof(struct txpd) + 4,

+ BUILD_BUG_ON(sizeof(txpd->tx_dest_addr) != ETH_ALEN);

+ memcpy(&txpd->tx_dest_addr, skb->data + sizeof(struct txpd) + 4,
ETH_ALEN);
txpd->tx_packet_length = cpu_to_le16(len);
txpd->tx_packet_location = cpu_to_le32(sizeof(struct txpd));
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index b1b51bbba054..54c014d97db7 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -204,7 +204,7 @@ static inline int mlx5e_get_max_num_channels(struct mlx5_core_dev *mdev)
struct mlx5e_tx_wqe {
struct mlx5_wqe_ctrl_seg ctrl;
struct mlx5_wqe_eth_seg eth;
- struct mlx5_wqe_data_seg data[0];
+ struct mlx5_wqe_data_seg data[];
};

struct mlx5e_rx_wqe_ll {
@@ -220,7 +220,7 @@ struct mlx5e_umr_wqe {
struct mlx5_wqe_ctrl_seg ctrl;
struct mlx5_wqe_umr_ctrl_seg uctrl;
struct mlx5_mkey_seg mkc;
- struct mlx5_mtt inline_mtts[0];
+ struct mlx5_mtt inline_mtts[];
};

extern const char mlx5e_self_tests[][ETH_GSTRING_LEN];
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
index 2f0df5cc1a2d..2d2364ea13cf 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
@@ -341,8 +341,10 @@ mlx5e_xmit_xdp_frame(struct mlx5e_xdpsq *sq, struct mlx5e_xmit_data *xdptxd,

/* copy the inline part if required */
if (sq->min_inline_mode != MLX5_INLINE_MODE_NONE) {
- memcpy(eseg->inline_hdr.start, xdptxd->data, MLX5E_XDP_MIN_INLINE);
+ memcpy(eseg->inline_hdr.start, xdptxd->data, sizeof(eseg->inline_hdr.start));
eseg->inline_hdr.sz = cpu_to_be16(MLX5E_XDP_MIN_INLINE);
+ memcpy(dseg, xdptxd->data + sizeof(eseg->inline_hdr.start),
+ MLX5E_XDP_MIN_INLINE - sizeof(eseg->inline_hdr.start));
dma_len -= MLX5E_XDP_MIN_INLINE;
dma_addr += MLX5E_XDP_MIN_INLINE;
dseg++;
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/net/wireless/intel/ipw2x00/libipw.h | 12 +++++++-----
drivers/net/wireless/intel/ipw2x00/libipw_rx.c | 8 ++++----
2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/intel/ipw2x00/libipw.h b/drivers/net/wireless/intel/ipw2x00/libipw.h
index 7964ef7d15f0..4006a0db2eea 100644
--- a/drivers/net/wireless/intel/ipw2x00/libipw.h
+++ b/drivers/net/wireless/intel/ipw2x00/libipw.h
@@ -537,11 +537,13 @@ struct libipw_txb {
struct libipw_qos_information_element {

u8 elementID;
u8 length;
- u8 qui[QOS_OUI_LEN];
- u8 qui_type;
- u8 qui_subtype;
- u8 version;
- u8 ac_info;
+ struct_group(data,
+ u8 qui[QOS_OUI_LEN];
+ u8 qui_type;
+ u8 qui_subtype;
+ u8 version;
+ u8 ac_info;
+ );
} __packed;

struct libipw_qos_ac_parameter {
diff --git a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
index 5a2a723e480b..75cc3cab4992 100644
--- a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
+++ b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
@@ -948,13 +948,13 @@ static int libipw_read_qos_param_element(struct libipw_qos_parameter_info

*info_element)
{
int ret = 0;

- u16 size = sizeof(struct libipw_qos_parameter_info) - 2;

+ u16 size = sizeof(element_param->info_element.data);

if ((info_element == NULL) || (element_param == NULL))

return -1;

if (info_element->id == QOS_ELEMENT_ID && info_element->len == size) {
- memcpy(element_param->info_element.qui, info_element->data,
+ memcpy(&element_param->info_element.data, info_element->data,
info_element->len);
element_param->info_element.elementID = info_element->id;
element_param->info_element.length = info_element->len;

@@ -975,7 +975,7 @@ static int libipw_read_qos_info_element(struct

*info_element)
{
int ret = 0;

- u16 size = sizeof(struct libipw_qos_information_element) - 2;

+ u16 size = sizeof(element_info->data);

if (element_info == NULL)
return -1;

@@ -983,7 +983,7 @@ static int libipw_read_qos_info_element(struct

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/net/wireless/marvell/mwl8k.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwl8k.c b/drivers/net/wireless/marvell/mwl8k.c
index 3bf6571f4149..a29277d5f9da 100644
--- a/drivers/net/wireless/marvell/mwl8k.c
+++ b/drivers/net/wireless/marvell/mwl8k.c
@@ -4225,9 +4225,11 @@ struct mwl8k_cmd_set_key {
__le32 key_info;
__le32 key_id;
__le16 key_len;
- __u8 key_material[MAX_ENCR_KEY_LENGTH];
- __u8 tkip_tx_mic_key[MIC_KEY_LENGTH];
- __u8 tkip_rx_mic_key[MIC_KEY_LENGTH];
+ struct {
+ __u8 key_material[MAX_ENCR_KEY_LENGTH];
+ __u8 tkip_tx_mic_key[MIC_KEY_LENGTH];
+ __u8 tkip_rx_mic_key[MIC_KEY_LENGTH];
+ } tkip;
__le16 tkip_rsc_low;
__le32 tkip_rsc_high;
__le16 tkip_tsc_low;
@@ -4375,7 +4377,7 @@ static int mwl8k_cmd_encryption_set_key(struct ieee80211_hw *hw,
goto done;
}

- memcpy(cmd->key_material, key->key, keymlen);
+ memcpy(&cmd->tkip, key->key, keymlen);
cmd->action = cpu_to_le32(action);

rc = mwl8k_post_pervif_cmd(hw, vif, &cmd->header);
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/hid/hid-cp2112.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 477baa30889c..e6ee453c7cfc 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -129,10 +129,12 @@ struct cp2112_xfer_status_report {

struct cp2112_string_report {
u8 dummy; /* force .string to be aligned */
- u8 report; /* CP2112_*_STRING */
- u8 length; /* length in bytes of everyting after .report */
- u8 type; /* USB_DT_STRING */
- wchar_t string[30]; /* UTF16_LITTLE_ENDIAN string */
+ struct_group_attr(contents, __packed,
+ u8 report; /* CP2112_*_STRING */
+ u8 length; /* length in bytes of everyting after .report */
+ u8 type; /* USB_DT_STRING */
+ wchar_t string[30]; /* UTF16_LITTLE_ENDIAN string */
+ );
} __packed;

/* Number of times to request transfer status before giving up waiting for a
@@ -986,8 +988,8 @@ static ssize_t pstr_show(struct device *kdev,
u8 length;
int ret;

- ret = cp2112_hid_get(hdev, attr->report, &report.report,
- sizeof(report) - 1, HID_FEATURE_REPORT);
+ ret = cp2112_hid_get(hdev, attr->report, (u8 *)&report.contents,
+ sizeof(report.contents), HID_FEATURE_REPORT);
if (ret < 3) {
hid_err(hdev, "error reading %s string: %d\n", kattr->attr.name,
ret);
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/net/ethernet/chelsio/cxgb3/sge.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb3/sge.c b/drivers/net/ethernet/chelsio/cxgb3/sge.c
index cb5c79c43bc9..1ab1bd86a3a6 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/sge.c
@@ -126,8 +126,10 @@ struct rsp_desc { /* response queue descriptor */
struct rss_header rss_hdr;
__be32 flags;
__be32 len_cq;
- u8 imm_data[47];
- u8 intr_gen;
+ struct_group(immediate,
+ u8 imm_data[47];
+ u8 intr_gen;
+ );
};

/*
@@ -929,7 +931,8 @@ static inline struct sk_buff *get_imm_packet(const struct rsp_desc *resp)

if (skb) {
__skb_put(skb, IMMED_PKT_SIZE);
- skb_copy_to_linear_data(skb, resp->imm_data, IMMED_PKT_SIZE);
+ BUILD_BUG_ON(IMMED_PKT_SIZE != sizeof(resp->immediate));
+ skb_copy_to_linear_data(skb, &resp->immediate, IMMED_PKT_SIZE);
}
return skb;
}
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +-
include/linux/if_vlan.h | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index c63d78eda606..39942a952736 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -207,7 +207,7 @@ static inline void mlx5e_insert_vlan(void *start, struct sk_buff *skb, u16 ihs)
int cpy1_sz = 2 * ETH_ALEN;
int cpy2_sz = ihs - cpy1_sz;

- memcpy(vhdr, skb->data, cpy1_sz);
+ memcpy(&vhdr->addrs, skb->data, cpy1_sz);
vhdr->h_vlan_proto = skb->vlan_proto;
vhdr->h_vlan_TCI = cpu_to_be16(skb_vlan_tag_get(skb));
memcpy(&vhdr->h_vlan_encapsulated_proto, skb->data + cpy1_sz, cpy2_sz);
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 41a518336673..45aad461aa34 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -46,8 +46,10 @@ struct vlan_hdr {
* @h_vlan_encapsulated_proto: packet type ID or len
*/
struct vlan_ethhdr {
- unsigned char h_dest[ETH_ALEN];
- unsigned char h_source[ETH_ALEN];
+ struct_group(addrs,
+ unsigned char h_dest[ETH_ALEN];
+ unsigned char h_source[ETH_ALEN];
+ );
__be16 h_vlan_proto;
__be16 h_vlan_TCI;
__be16 h_vlan_encapsulated_proto;
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/arch/s390/lib/string.c b/arch/s390/lib/string.c
index cfcdf76d6a95..392fb9f4f4db 100644
--- a/arch/s390/lib/string.c
+++ b/arch/s390/lib/string.c
@@ -8,6 +8,9 @@
*/

#define IN_ARCH_STRING_C 1
+#ifndef __NO_FORTIFY
+# define __NO_FORTIFY
+#endif

#include <linux/types.h>
#include <linux/string.h>
diff --git a/arch/x86/lib/string_32.c b/arch/x86/lib/string_32.c
index d15fdae9656e..53b3f202267c 100644
--- a/arch/x86/lib/string_32.c
+++ b/arch/x86/lib/string_32.c
@@ -11,6 +11,7 @@
* strings.
*/

+#define __NO_FORTIFY
#include <linux/string.h>
#include <linux/export.h>

diff --git a/lib/string.c b/lib/string.c
index 4fec38fc6e58..4e111d9dd6d5 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -6,20 +6,15 @@
*/

/*
- * stupid library routines.. The optimized versions should generally be found
- * as inline code in <asm-xx/string.h>
+ * This file should be used only for "library" routines that may have
+ * alternative implementations on specific architectures (generally
+ * found in <asm-xx/string.h>), or get overloaded by FORTIFY_SOURCE.
+ * (Specifically, this file is built with __NO_FORTIFY.)
*
- * These are buggy as well..
- *
- * * Fri Jun 25 1999, Ingo Oeser <i...@informatik.tu-chemnitz.de>
- * - Added strsep() which will replace strtok() soon (because strsep() is
- * reentrant and should be faster). Use only strsep() in new code, please.
- *
- * * Sat Feb 09 2002, Jason Thomas <ja...@topic.com.au>,
- * Matthew Hawkins <ma...@mh.dropbear.id.au>
- * - Kissed strtok() goodbye
+ * Other helper functions should live in string_helpers.c.
*/

+#define __NO_FORTIFY
#include <linux/types.h>
#include <linux/string.h>
#include <linux/ctype.h>
@@ -254,40 +249,6 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
EXPORT_SYMBOL(strscpy);
#endif

-/**
- * strscpy_pad() - Copy a C-string into a sized buffer
- * @dest: Where to copy the string to
- * @src: Where to copy the string from
- * @count: Size of destination buffer
- *
- * Copy the string, or as much of it as fits, into the dest buffer. The
- * behavior is undefined if the string buffers overlap. The destination
- * buffer is always %NUL terminated, unless it's zero-sized.
- *
- * If the source string is shorter than the destination buffer, zeros
- * the tail of the destination buffer.
- *
- * For full explanation of why you may want to consider using the
- * 'strscpy' functions please see the function docstring for strscpy().
- *
- * Returns:
- * * The number of characters copied (not including the trailing %NUL)
- * * -E2BIG if count is 0 or @src was truncated.
- */
-ssize_t strscpy_pad(char *dest, const char *src, size_t count)
-{
- ssize_t written;
-
- written = strscpy(dest, src, count);
- if (written < 0 || written == count - 1)
- return written;
-
- memset(dest + written + 1, 0, count - written - 1);
-
- return written;
-}
-EXPORT_SYMBOL(strscpy_pad);
-
/**
* stpcpy - copy a string from src to dest returning a pointer to the new end
* of dest, including src's %NUL-terminator. May overrun dest.
@@ -530,46 +491,6 @@ char *strnchr(const char *s, size_t count, int c)
EXPORT_SYMBOL(strnchr);
#endif

-/**
- * skip_spaces - Removes leading whitespace from @str.
- * @str: The string to be stripped.
- *
- * Returns a pointer to the first non-whitespace character in @str.
- */
-char *skip_spaces(const char *str)
-{
- while (isspace(*str))
- ++str;
- return (char *)str;
-}
-EXPORT_SYMBOL(skip_spaces);
-
-/**
- * strim - Removes leading and trailing whitespace from @s.
- * @s: The string to be stripped.
- *
- * Note that the first trailing whitespace is replaced with a %NUL-terminator
- * in the given string @s. Returns a pointer to the first non-whitespace
- * character in @s.
- */
-char *strim(char *s)
-{
- size_t size;
- char *end;
-
- size = strlen(s);
- if (!size)
- return s;
-
- end = s + size - 1;
- while (end >= s && isspace(*end))
- end--;
- *(end + 1) = '\0';
-
- return skip_spaces(s);
-}
-EXPORT_SYMBOL(strim);
-
#ifndef __HAVE_ARCH_STRLEN
/**
* strlen - Find the length of a string
@@ -704,101 +625,6 @@ char *strsep(char **s, const char *ct)
EXPORT_SYMBOL(strsep);
#endif

-/**
- * sysfs_streq - return true if strings are equal, modulo trailing newline
- * @s1: one string
- * @s2: another string
- *
- * This routine returns true iff two strings are equal, treating both
- * NUL and newline-then-NUL as equivalent string terminations. It's
- * geared for use with sysfs input strings, which generally terminate
- * with newlines but are compared against values without newlines.
- */
-bool sysfs_streq(const char *s1, const char *s2)
-{
- while (*s1 && *s1 == *s2) {
- s1++;
- s2++;
- }
-
- if (*s1 == *s2)
- return true;
- if (!*s1 && *s2 == '\n' && !s2[1])
- return true;
- if (*s1 == '\n' && !s1[1] && !*s2)
- return true;
- return false;
-}
-EXPORT_SYMBOL(sysfs_streq);
-
-/**
- * match_string - matches given string in an array
- * @array: array of strings
- * @n: number of strings in the array or -1 for NULL terminated arrays
- * @string: string to match with
- *
- * This routine will look for a string in an array of strings up to the
- * n-th element in the array or until the first NULL element.
- *
- * Historically the value of -1 for @n, was used to search in arrays that
- * are NULL terminated. However, the function does not make a distinction
- * when finishing the search: either @n elements have been compared OR
- * the first NULL element was found.
- *
- * Return:
- * index of a @string in the @array if matches, or %-EINVAL otherwise.
- */
-int match_string(const char * const *array, size_t n, const char *string)
-{
- int index;
- const char *item;
-
- for (index = 0; index < n; index++) {
- item = array[index];
- if (!item)
- break;
- if (!strcmp(item, string))
- return index;
- }
-
- return -EINVAL;
-}
-EXPORT_SYMBOL(match_string);
-
-/**
- * __sysfs_match_string - matches given string in an array
- * @array: array of strings
- * @n: number of strings in the array or -1 for NULL terminated arrays
- * @str: string to match with
- *
- * Returns index of @str in the @array or -EINVAL, just like match_string().
- * Uses sysfs_streq instead of strcmp for matching.
- *
- * This routine will look for a string in an array of strings up to the
- * n-th element in the array or until the first NULL element.
- *
- * Historically the value of -1 for @n, was used to search in arrays that
- * are NULL terminated. However, the function does not make a distinction
- * when finishing the search: either @n elements have been compared OR
- * the first NULL element was found.
- */
-int __sysfs_match_string(const char * const *array, size_t n, const char *str)
-{
- const char *item;
- int index;
-
- for (index = 0; index < n; index++) {
- item = array[index];
- if (!item)
- break;
- if (sysfs_streq(item, str))
- return index;
- }
-
- return -EINVAL;
-}
-EXPORT_SYMBOL(__sysfs_match_string);
-
#ifndef __HAVE_ARCH_MEMSET
/**
* memset - Fill a region of memory with the given value
@@ -1221,27 +1047,3 @@ void *memchr_inv(const void *start, int c, size_t bytes)
return check_bytes8(start, value, bytes % 8);
}
EXPORT_SYMBOL(memchr_inv);
-
-/**
- * strreplace - Replace all occurrences of character in string.
- * @s: The string to operate on.
- * @old: The character being replaced.
- * @new: The character @old is replaced with.
- *
- * Returns pointer to the nul byte at the end of @s.
- */
-char *strreplace(char *s, char old, char new)
-{
- for (; *s; ++s)
- if (*s == old)
- *s = new;
- return s;
-}
-EXPORT_SYMBOL(strreplace);
-
-void fortify_panic(const char *name)
-{
- pr_emerg("detected buffer overflow in %s\n", name);
- BUG();
-}
-EXPORT_SYMBOL(fortify_panic);
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 3806a52ce697..bde13612c25d 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -696,3 +696,196 @@ void kfree_strarray(char **array, size_t n)
kfree(array);
}
EXPORT_SYMBOL_GPL(kfree_strarray);
+
+/**
+ * strscpy_pad() - Copy a C-string into a sized buffer
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ * @count: Size of destination buffer
+ *
+ * Copy the string, or as much of it as fits, into the dest buffer. The
+ * behavior is undefined if the string buffers overlap. The destination
+ * buffer is always %NUL terminated, unless it's zero-sized.
+ *
+ * If the source string is shorter than the destination buffer, zeros
+ * the tail of the destination buffer.
+ *
+ * For full explanation of why you may want to consider using the
+ * 'strscpy' functions please see the function docstring for strscpy().
+ *
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if count is 0 or @src was truncated.
+ */
+ssize_t strscpy_pad(char *dest, const char *src, size_t count)
+{
+ ssize_t written;
+
+ written = strscpy(dest, src, count);
+ if (written < 0 || written == count - 1)
+ return written;
+
+ memset(dest + written + 1, 0, count - written - 1);
+
+ return written;
+}
+EXPORT_SYMBOL(strscpy_pad);
+
+/**
+ * skip_spaces - Removes leading whitespace from @str.
+ * @str: The string to be stripped.
+ *
+ * Returns a pointer to the first non-whitespace character in @str.
+ */
+char *skip_spaces(const char *str)
+{
+ while (isspace(*str))
+ ++str;
+ return (char *)str;
+}
+EXPORT_SYMBOL(skip_spaces);
+
+/**
+ * strim - Removes leading and trailing whitespace from @s.
+ * @s: The string to be stripped.
+ *
+ * Note that the first trailing whitespace is replaced with a %NUL-terminator
+ * in the given string @s. Returns a pointer to the first non-whitespace
+ * character in @s.
+ */
+char *strim(char *s)
+{
+ size_t size;
+ char *end;
+
+ size = strlen(s);
+ if (!size)
+ return s;
+
+ end = s + size - 1;
+ while (end >= s && isspace(*end))
+ end--;
+ *(end + 1) = '\0';
+
+ return skip_spaces(s);
+}
+EXPORT_SYMBOL(strim);
+
+/**
+ * sysfs_streq - return true if strings are equal, modulo trailing newline
+ * @s1: one string
+ * @s2: another string
+ *
+ * This routine returns true iff two strings are equal, treating both
+ * NUL and newline-then-NUL as equivalent string terminations. It's
+ * geared for use with sysfs input strings, which generally terminate
+ * with newlines but are compared against values without newlines.
+ */
+bool sysfs_streq(const char *s1, const char *s2)
+{
+ while (*s1 && *s1 == *s2) {
+ s1++;
+ s2++;
+ }
+
+ if (*s1 == *s2)
+ return true;
+ if (!*s1 && *s2 == '\n' && !s2[1])
+ return true;
+ if (*s1 == '\n' && !s1[1] && !*s2)
+ return true;
+ return false;
+}
+EXPORT_SYMBOL(sysfs_streq);
+
+/**
+ * match_string - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @string: string to match with
+ *
+ * This routine will look for a string in an array of strings up to the
+ * n-th element in the array or until the first NULL element.
+ *
+ * Historically the value of -1 for @n, was used to search in arrays that
+ * are NULL terminated. However, the function does not make a distinction
+ * when finishing the search: either @n elements have been compared OR
+ * the first NULL element was found.
+ *
+ * Return:
+ * index of a @string in the @array if matches, or %-EINVAL otherwise.
+ */
+int match_string(const char * const *array, size_t n, const char *string)
+{
+ int index;
+ const char *item;
+
+ for (index = 0; index < n; index++) {
+ item = array[index];
+ if (!item)
+ break;
+ if (!strcmp(item, string))
+ return index;
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(match_string);
+
+/**
+ * __sysfs_match_string - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq instead of strcmp for matching.
+ *
+ * This routine will look for a string in an array of strings up to the
+ * n-th element in the array or until the first NULL element.
+ *
+ * Historically the value of -1 for @n, was used to search in arrays that
+ * are NULL terminated. However, the function does not make a distinction
+ * when finishing the search: either @n elements have been compared OR
+ * the first NULL element was found.
+ */
+int __sysfs_match_string(const char * const *array, size_t n, const char *str)
+{
+ const char *item;
+ int index;
+
+ for (index = 0; index < n; index++) {
+ item = array[index];
+ if (!item)
+ break;
+ if (sysfs_streq(item, str))
+ return index;
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(__sysfs_match_string);
+
+/**
+ * strreplace - Replace all occurrences of character in string.
+ * @s: The string to operate on.
+ * @old: The character being replaced.
+ * @new: The character @old is replaced with.
+ *
+ * Returns pointer to the nul byte at the end of @s.
+ */
+char *strreplace(char *s, char old, char new)
+{
+ for (; *s; ++s)
+ if (*s == old)
+ *s = new;
+ return s;
+}
+EXPORT_SYMBOL(strreplace);
+
+void fortify_panic(const char *name)
+{
+ pr_emerg("detected buffer overflow in %s\n", name);
+ BUG();
+}
+EXPORT_SYMBOL(fortify_panic);
--
2.30.2

[Kees Cook]

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 <ndesau...@google.com>

Signed-off-by: Kees Cook <kees...@chromium.org>
---

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

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index cb9217fc60af..01985821944b 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -41,8 +41,6 @@

#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)

-#define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
-
#define __compiletime_warning(message) __attribute__((__warning__(message)))
#define __compiletime_error(message) __attribute__((__error__(message)))

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index e4ea86fc584d..c43308b0a9a9 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -290,10 +290,6 @@ struct ftrace_likely_data {
(sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))

-/* Compile time object size, -1 for unknown */
-#ifndef __compiletime_object_size
-# define __compiletime_object_size(obj) -1
-#endif
#ifndef __compiletime_warning
# define __compiletime_warning(message)
#endif
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 0999f6317978..ad0c4e041030 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -203,7 +203,7 @@ static inline void copy_overflow(int size, unsigned long count)
static __always_inline __must_check bool
check_copy_size(const void *addr, size_t bytes, bool is_source)
{
- int sz = __compiletime_object_size(addr);
+ int sz = __builtin_object_size(addr, 0);
if (unlikely(sz >= 0 && sz < bytes)) {
if (!__builtin_constant_p(bytes))
copy_overflow(sz, bytes);
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

include/linux/fortify-string.h | 7 +++++++
include/linux/string.h | 9 ---------
lib/string_helpers.c | 2 ++
3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index c1be37437e77..7e67d02764db 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -2,6 +2,13 @@
#ifndef _LINUX_FORTIFY_STRING_H_
#define _LINUX_FORTIFY_STRING_H_

+#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 __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");

#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);
diff --git a/include/linux/string.h b/include/linux/string.h
index b48d2d28e0b1..9473f81b9db2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -249,15 +249,6 @@ static inline const char *kbasename(const char *path)
return tail ? tail + 1 : path;
}

-#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 passed as 1st parameter");
-void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter");
-void __read_overflow3(void) __compiletime_error("detected read beyond size of object passed as 3rd parameter");
-void __write_overflow(void) __compiletime_error("detected write beyond size of object passed as 1st parameter");
-
#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
#include <linux/fortify-string.h>
#endif
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index bde13612c25d..faa9d8e4e2c5 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -883,9 +883,11 @@ char *strreplace(char *s, char old, char new)
}
EXPORT_SYMBOL(strreplace);

+#ifdef CONFIG_FORTIFY_SOURCE
void fortify_panic(const char *name)
{

pr_emerg("detected buffer overflow in %s\n", name);

BUG();
}
EXPORT_SYMBOL(fortify_panic);
+#endif /* CONFIG_FORTIFY_SOURCE */
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

security/Kconfig | 3 +++
1 file changed, 3 insertions(+)

diff --git a/security/Kconfig b/security/Kconfig
index 0ced7fd33e4d..8f0e675e70a4 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -191,6 +191,9 @@ config HARDENED_USERCOPY_PAGESPAN
config FORTIFY_SOURCE
bool "Harden common str/mem functions against buffer overflows"
depends on ARCH_HAS_FORTIFY_SOURCE
+ # https://bugs.llvm.org/show_bug.cgi?id=50322
+ # https://bugs.llvm.org/show_bug.cgi?id=41459
+ depends on !CONFIG_CC_IS_CLANG
help
Detect overflows of buffers in common string and memory functions
where the compiler can determine and validate the buffer sizes.
--
2.30.2

[Kees Cook]

While the run-time testing of FORTIFY_SOURCE is already present in
LKDTM, there is no testing of the expected compile-time detections. In
preparation for correctly supporting FORTIFY_SOURCE under Clang, adding
additional FORTIFY_SOURCE defenses, and making sure FORTIFY_SOURCE
doesn't silently regress with GCC, introduce a build-time test suite that
checks each expected compile-time failure condition.

As this is relatively backwards from standard build rules in the
sense that a successful test is actually a compile _failure_, create
a wrapper script to check for the correct errors, and wire it up as
a dummy dependency to lib/string.o, collecting the results into a log
file artifact.

Signed-off-by: Kees Cook <kees...@chromium.org>
---

lib/.gitignore | 2 +
lib/Makefile | 30 ++++++++++
lib/test_fortify/read_overflow-memchr.c | 5 ++
lib/test_fortify/read_overflow-memchr_inv.c | 5 ++
lib/test_fortify/read_overflow-memcmp.c | 5 ++
lib/test_fortify/read_overflow-memscan.c | 5 ++
lib/test_fortify/read_overflow2-memcmp.c | 5 ++
lib/test_fortify/read_overflow2-memcpy.c | 5 ++
lib/test_fortify/read_overflow2-memmove.c | 5 ++
lib/test_fortify/test_fortify.h | 31 ++++++++++
lib/test_fortify/write_overflow-memcpy.c | 5 ++
lib/test_fortify/write_overflow-memmove.c | 5 ++
lib/test_fortify/write_overflow-memset.c | 5 ++
lib/test_fortify/write_overflow-strlcpy.c | 5 ++
lib/test_fortify/write_overflow-strncpy.c | 5 ++
lib/test_fortify/write_overflow-strscpy.c | 5 ++
scripts/test_fortify.sh | 64 +++++++++++++++++++++
17 files changed, 192 insertions(+)

create mode 100644 lib/test_fortify/read_overflow-memchr.c
create mode 100644 lib/test_fortify/read_overflow-memchr_inv.c
create mode 100644 lib/test_fortify/read_overflow-memcmp.c
create mode 100644 lib/test_fortify/read_overflow-memscan.c
create mode 100644 lib/test_fortify/read_overflow2-memcmp.c
create mode 100644 lib/test_fortify/read_overflow2-memcpy.c
create mode 100644 lib/test_fortify/read_overflow2-memmove.c

create mode 100644 lib/test_fortify/test_fortify.h
create mode 100644 lib/test_fortify/write_overflow-memcpy.c
create mode 100644 lib/test_fortify/write_overflow-memmove.c
create mode 100644 lib/test_fortify/write_overflow-memset.c
create mode 100644 lib/test_fortify/write_overflow-strlcpy.c
create mode 100644 lib/test_fortify/write_overflow-strncpy.c
create mode 100644 lib/test_fortify/write_overflow-strscpy.c

create mode 100644 scripts/test_fortify.sh

diff --git a/lib/.gitignore b/lib/.gitignore
index 5e7fa54c4536..e5e217b8307b 100644
--- a/lib/.gitignore
+++ b/lib/.gitignore
@@ -4,3 +4,5 @@
/gen_crc32table
/gen_crc64table
/oid_registry_data.c
+/test_fortify.log
+/test_fortify/*.log
diff --git a/lib/Makefile b/lib/Makefile
index 5efd1b435a37..40b4bf0bc847 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -360,3 +360,33 @@ obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o

obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
+
+# FORTIFY_SOURCE compile-time behavior tests
+TEST_FORTIFY_SRCS = $(wildcard $(srctree)/$(src)/test_fortify/*-*.c)
+TEST_FORTIFY_LOGS = $(patsubst $(srctree)/$(src)/%.c, %.log, $(TEST_FORTIFY_SRCS))
+TEST_FORTIFY_LOG = test_fortify.log
+
+quiet_cmd_test_fortify = TEST $@
+ cmd_test_fortify = $(CONFIG_SHELL) $(srctree)/scripts/test_fortify.sh \
+ $< $@ "$(NM)" $(CC) $(c_flags) \
+ $(call cc-disable-warning,fortify-source)
+
+targets += $(TEST_FORTIFY_LOGS)
+clean-files += $(TEST_FORTIFY_LOGS)
+clean-files += $(addsuffix .o, $(TEST_FORTIFY_LOGS))
+$(obj)/test_fortify/%.log: $(src)/test_fortify/%.c $(srctree)/scripts/test_fortify.sh \
+ $(srctree)/include/linux/fortify-string.h FORCE
+ $(call if_changed,test_fortify)
+
+quiet_cmd_gen_fortify_log = GEN $@
+ cmd_gen_fortify_log = cat </dev/null $(filter-out FORCE,$^) 2>/dev/null > $@ || true
+
+targets += $(TEST_FORTIFY_LOG)
+clean-files += $(TEST_FORTIFY_LOG)
+$(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE
+ $(call if_changed,gen_fortify_log)
+
+# Fake dependency to trigger the fortify tests.
+ifeq ($(CONFIG_FORTIFY_SOURCE),y)
+$(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG)
+endif
diff --git a/lib/test_fortify/read_overflow-memchr.c b/lib/test_fortify/read_overflow-memchr.c
new file mode 100644
index 000000000000..2743084b32af
--- /dev/null
+++ b/lib/test_fortify/read_overflow-memchr.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memchr(small, 0x7A, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow-memchr_inv.c b/lib/test_fortify/read_overflow-memchr_inv.c
new file mode 100644
index 000000000000..b26e1f1bc217
--- /dev/null
+++ b/lib/test_fortify/read_overflow-memchr_inv.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memchr_inv(small, 0x7A, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow-memcmp.c b/lib/test_fortify/read_overflow-memcmp.c
new file mode 100644
index 000000000000..d5d301ff64ef
--- /dev/null
+++ b/lib/test_fortify/read_overflow-memcmp.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memcmp(small, large, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow-memscan.c b/lib/test_fortify/read_overflow-memscan.c
new file mode 100644
index 000000000000..c1a97f2df0f0
--- /dev/null
+++ b/lib/test_fortify/read_overflow-memscan.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memscan(small, 0x7A, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow2-memcmp.c b/lib/test_fortify/read_overflow2-memcmp.c
new file mode 100644
index 000000000000..c6091e640f76
--- /dev/null
+++ b/lib/test_fortify/read_overflow2-memcmp.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memcmp(large, small, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow2-memcpy.c b/lib/test_fortify/read_overflow2-memcpy.c
new file mode 100644
index 000000000000..07b62e56cf16
--- /dev/null
+++ b/lib/test_fortify/read_overflow2-memcpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memcpy(large, instance.buf, sizeof(large))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow2-memmove.c b/lib/test_fortify/read_overflow2-memmove.c
new file mode 100644
index 000000000000..34edfab040a3
--- /dev/null
+++ b/lib/test_fortify/read_overflow2-memmove.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memmove(large, instance.buf, sizeof(large))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/test_fortify.h b/lib/test_fortify/test_fortify.h
new file mode 100644
index 000000000000..125f5c585e4f
--- /dev/null
+++ b/lib/test_fortify/test_fortify.h
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/kernel.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+void do_fortify_tests(void);
+
+# define __BUF_SMALL 16
+# define __BUF_LARGE 32
+struct fortify_object {
+ int a;
+ char buf[__BUF_SMALL];
+ int c;
+};
+const char small_src[__BUF_SMALL] = "AAAAAAAAAAAAAAA";
+const char large_src[__BUF_LARGE] = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
+
+char small[__BUF_SMALL];
+char large[__BUF_LARGE];
+struct fortify_object instance;
+
+void do_fortify_tests(void)
+{
+ /* Normal initializations. */
+ memset(&instance, 0x32, sizeof(instance));
+ memset(small, 0xA5, sizeof(small));
+ memset(large, 0x5A, sizeof(large));
+
+ TEST;
+}
diff --git a/lib/test_fortify/write_overflow-memcpy.c b/lib/test_fortify/write_overflow-memcpy.c
new file mode 100644
index 000000000000..3b3984e428fb
--- /dev/null
+++ b/lib/test_fortify/write_overflow-memcpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memcpy(instance.buf, large_src, sizeof(large_src))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-memmove.c b/lib/test_fortify/write_overflow-memmove.c
new file mode 100644
index 000000000000..640437c3b3e0
--- /dev/null
+++ b/lib/test_fortify/write_overflow-memmove.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memmove(instance.buf, large_src, sizeof(large_src))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-memset.c b/lib/test_fortify/write_overflow-memset.c
new file mode 100644
index 000000000000..36e34908cfb3
--- /dev/null
+++ b/lib/test_fortify/write_overflow-memset.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memset(instance.buf, 0x5A, sizeof(large_src))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strlcpy.c b/lib/test_fortify/write_overflow-strlcpy.c
new file mode 100644
index 000000000000..1883db7c0cd6
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strlcpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strlcpy(instance.buf, large_src, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strncpy.c b/lib/test_fortify/write_overflow-strncpy.c
new file mode 100644
index 000000000000..b85f079c815d
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strncpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strncpy(instance.buf, large_src, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strscpy.c b/lib/test_fortify/write_overflow-strscpy.c
new file mode 100644
index 000000000000..38feddf377dc
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strscpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strscpy(instance.buf, large_src, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
diff --git a/scripts/test_fortify.sh b/scripts/test_fortify.sh
new file mode 100644
index 000000000000..59b362eed2ac
--- /dev/null
+++ b/scripts/test_fortify.sh
@@ -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"
+ exit 0
+fi
+
+# Clean up temporary file at exit.
+__cleanup() {
+ rm -f "$TMP"
+}
+trap __cleanup EXIT
+
+status=
+# Attempt to build a source that is expected to fail with a specific warning.
+if "$@" -Werror -c "$IN" -o "$OUT".o 2> "$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
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

fs/btrfs/root-tree.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 702dc5441f03..ec9e78f65fca 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -39,10 +39,7 @@ static void btrfs_read_root_item(struct extent_buffer *eb, int slot,
need_reset = 1;
}
if (need_reset) {
- memset(&item->generation_v2, 0,
- sizeof(*item) - offsetof(struct btrfs_root_item,
- generation_v2));
-
+ memset_after(item, 0, level);
generate_random_guid(item->uuid);
}
}
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

include/linux/fortify-string.h | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 4afd42079d3b..0d0acd959ba0 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -260,7 +260,7 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
* V = vulnerable to run-time overflow
*
*/
-__FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
+__FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
const size_t p_size,
const size_t q_size,
const size_t p_size_field,
@@ -309,13 +309,25 @@ __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
if ((p_size != (size_t)(-1) && p_size < size) ||
(q_size != (size_t)(-1) && q_size < size))
fortify_panic(func);
+
+ /*
+ * Warn when writing beyond destination field size. Since
+ * flexible-arrays are considered 0 bytes, we must ignore 0 sizes
+ * at runtime for now.
+ */
+ if (p_size_field && p_size != p_size_field && p_size_field < size)
+ return true;
+
+ return false;
}

#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); \
+ WARN_ONCE(fortify_memcpy_chk(__fortify_size, p_size, q_size, \
+ p_size_field, q_size_field, #op), \
+ #op ": detected field-spanning write (size %zu) of single field (size %zu)\n", \
+ __fortify_size, p_size_field); \
__underlying_##op(p, q, __fortify_size); \
})

--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/gpu/drm/mga/mga_ioc32.c | 30 ++++++++++++++------------
include/uapi/drm/mga_drm.h | 37 ++++++++++++++++++++++++---------
2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/mga/mga_ioc32.c b/drivers/gpu/drm/mga/mga_ioc32.c
index 4fd4de16cd32..fbd0329dbd4f 100644
--- a/drivers/gpu/drm/mga/mga_ioc32.c
+++ b/drivers/gpu/drm/mga/mga_ioc32.c
@@ -38,16 +38,21 @@
typedef struct drm32_mga_init {
int func;
u32 sarea_priv_offset;
- int chipset;
- int sgram;
- unsigned int maccess;
- unsigned int fb_cpp;
- unsigned int front_offset, front_pitch;
- unsigned int back_offset, back_pitch;
- unsigned int depth_cpp;
- unsigned int depth_offset, depth_pitch;
- unsigned int texture_offset[MGA_NR_TEX_HEAPS];
- unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ struct_group(always32bit,
+ int chipset;
+ int sgram;
+ unsigned int maccess;
+ unsigned int fb_cpp;
+ unsigned int front_offset;
+ unsigned int front_pitch;
+ unsigned int back_offset;
+ unsigned int back_pitch;
+ unsigned int depth_cpp;
+ unsigned int depth_offset;
+ unsigned int depth_pitch;
+ unsigned int texture_offset[MGA_NR_TEX_HEAPS];
+ unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ );
u32 fb_offset;
u32 mmio_offset;
u32 status_offset;
@@ -67,9 +72,8 @@ static int compat_mga_init(struct file *file, unsigned int cmd,

init.func = init32.func;
init.sarea_priv_offset = init32.sarea_priv_offset;
- memcpy(&init.chipset, &init32.chipset,
- offsetof(drm_mga_init_t, fb_offset) -
- offsetof(drm_mga_init_t, chipset));
+ memcpy(&init.always32bit, &init32.always32bit,
+ sizeof(init32.always32bit));
init.fb_offset = init32.fb_offset;
init.mmio_offset = init32.mmio_offset;
init.status_offset = init32.status_offset;
diff --git a/include/uapi/drm/mga_drm.h b/include/uapi/drm/mga_drm.h
index 8c4337548ab5..61612e5ecab2 100644
--- a/include/uapi/drm/mga_drm.h
+++ b/include/uapi/drm/mga_drm.h
@@ -265,6 +265,16 @@ typedef struct _drm_mga_sarea {
#define DRM_IOCTL_MGA_WAIT_FENCE DRM_IOWR(DRM_COMMAND_BASE + DRM_MGA_WAIT_FENCE, __u32)
#define DRM_IOCTL_MGA_DMA_BOOTSTRAP DRM_IOWR(DRM_COMMAND_BASE + DRM_MGA_DMA_BOOTSTRAP, drm_mga_dma_bootstrap_t)

+#define __struct_group(name, fields) \
+ union { \
+ struct { \
+ fields \
+ }; \
+ struct { \
+ fields \
+ } name; \
+ }
+
typedef struct _drm_mga_warp_index {
int installed;
unsigned long phys_addr;
@@ -279,20 +289,25 @@ typedef struct drm_mga_init {

unsigned long sarea_priv_offset;

- int chipset;
- int sgram;
+ __struct_group(always32bit,
+ int chipset;
+ int sgram;

- unsigned int maccess;
+ unsigned int maccess;

- unsigned int fb_cpp;
- unsigned int front_offset, front_pitch;
- unsigned int back_offset, back_pitch;
+ unsigned int fb_cpp;
+ unsigned int front_offset;
+ unsigned int front_pitch;
+ unsigned int back_offset;
+ unsigned int back_pitch;

- unsigned int depth_cpp;
- unsigned int depth_offset, depth_pitch;
+ unsigned int depth_cpp;
+ unsigned int depth_offset;
+ unsigned int depth_pitch;

- unsigned int texture_offset[MGA_NR_TEX_HEAPS];
- unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ unsigned int texture_offset[MGA_NR_TEX_HEAPS];
+ unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ );

unsigned long fb_offset;
unsigned long mmio_offset;
@@ -302,6 +317,8 @@ typedef struct drm_mga_init {
unsigned long buffers_offset;
} drm_mga_init_t;

+#undef __struct_group
+
typedef struct drm_mga_dma_bootstrap {
/**
* \name AGP texture region
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/include/net/flow.h b/include/net/flow.h
index 6f5e70240071..f1a3b6c8eae2 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -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 */
- __be32 saddr;
- __be32 daddr;
+ struct_group(addrs,
+ __be32 saddr;
+ __be32 daddr;
+ );

union flowi_uli uli;
#define fl4_sport uli.ports.sport
diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
index a0b637911d3c..8f5667b2ea92 100644
--- a/include/uapi/linux/if_ether.h
+++ b/include/uapi/linux/if_ether.h
@@ -163,8 +163,16 @@

#if __UAPI_DEF_ETHHDR
struct ethhdr {
- unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
- unsigned char h_source[ETH_ALEN]; /* source ether addr */
+ union {
+ struct {
+ unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
+ unsigned char h_source[ETH_ALEN]; /* source ether addr */
+ };
+ struct {
+ unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
+ unsigned char h_source[ETH_ALEN]; /* source ether addr */
+ } addrs;
+ };
__be16 h_proto; /* packet type ID field */
} __attribute__((packed));
#endif
diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
index e42d13b55cf3..33647a37e56b 100644
--- a/include/uapi/linux/ip.h
+++ b/include/uapi/linux/ip.h
@@ -100,8 +100,16 @@ struct iphdr {
__u8 ttl;
__u8 protocol;
__sum16 check;
- __be32 saddr;
- __be32 daddr;
+ union {
+ struct {
+ __be32 saddr;
+ __be32 daddr;
+ } addrs;
+ struct {
+ __be32 saddr;
+ __be32 daddr;
+ };
+ };
/*The options start here. */
};

diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
index b243a53fa985..1c26d32e733b 100644
--- a/include/uapi/linux/ipv6.h
+++ b/include/uapi/linux/ipv6.h
@@ -130,8 +130,16 @@ struct ipv6hdr {
__u8 nexthdr;
__u8 hop_limit;

- struct in6_addr saddr;
- struct in6_addr daddr;
+ union {
+ struct {
+ struct in6_addr saddr;
+ struct in6_addr daddr;
+ } addrs;
+ struct {
+ struct in6_addr saddr;
+ struct in6_addr daddr;
+ };
+ };
};


diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 2aadbfc5193b..87655a2ac200 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1029,7 +1029,8 @@ bool __skb_flow_dissect(const struct net *net,
key_eth_addrs = skb_flow_dissector_target(flow_dissector,
FLOW_DISSECTOR_KEY_ETH_ADDRS,
target_container);
- memcpy(key_eth_addrs, &eth->h_dest, sizeof(*key_eth_addrs));
+ BUILD_BUG_ON(sizeof(*key_eth_addrs) != sizeof(eth->addrs));
+ memcpy(key_eth_addrs, &eth->addrs, sizeof(*key_eth_addrs));
}

proto_again:
@@ -1056,8 +1057,8 @@ bool __skb_flow_dissect(const struct net *net,
FLOW_DISSECTOR_KEY_IPV4_ADDRS,
target_container);

- memcpy(&key_addrs->v4addrs, &iph->saddr,
- sizeof(key_addrs->v4addrs));
+ BUILD_BUG_ON(sizeof(key_addrs->v4addrs) != sizeof(iph->addrs));
+ memcpy(&key_addrs->v4addrs, &iph->addrs, sizeof(iph->addrs));
key_control->addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
}

@@ -1101,7 +1102,8 @@ bool __skb_flow_dissect(const struct net *net,
FLOW_DISSECTOR_KEY_IPV6_ADDRS,
target_container);

- memcpy(&key_addrs->v6addrs, &iph->saddr,
+ BUILD_BUG_ON(sizeof(iph->addrs) != sizeof(key_addrs->v6addrs));
+ memcpy(&key_addrs->v6addrs, &iph->addrs,
sizeof(key_addrs->v6addrs));
key_control->addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
}
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 8d8a8da3ae7e..58603995d889 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -444,10 +444,8 @@ EXPORT_SYMBOL(ip_output);
*/
static void ip_copy_addrs(struct iphdr *iph, const struct flowi4 *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 */
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

net/802/hippi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/802/hippi.c b/net/802/hippi.c
index f80b33a8f7e0..00f6666b4b16 100644
--- a/net/802/hippi.c
+++ b/net/802/hippi.c
@@ -65,7 +65,7 @@ static int hippi_header(struct sk_buff *skb, struct net_device *dev,
hip->le.src_addr_type = 2; /* 12 bit SC address */

memcpy(hip->le.src_switch_addr, dev->dev_addr + 3, 3);
- memset(&hip->le.reserved, 0, 16);
+ memset_after(&hip->le, 0, src_switch_addr);

hip->snap.dsap = HIPPI_EXTENDED_SAP;
hip->snap.ssap = HIPPI_EXTENDED_SAP;
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

.../intel/int340x_thermal/acpi_thermal_rel.c | 5 +-
.../intel/int340x_thermal/acpi_thermal_rel.h | 48 ++++++++++---------
2 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
index a478cff8162a..e90690a234c4 100644
--- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
+++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
@@ -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))
diff --git a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
index 58822575fd54..78d942477035 100644
--- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
+++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
@@ -17,17 +17,19 @@
struct art {
acpi_handle source;
acpi_handle target;
- u64 weight;
- u64 ac0_max;
- u64 ac1_max;
- u64 ac2_max;
- u64 ac3_max;
- u64 ac4_max;
- u64 ac5_max;
- u64 ac6_max;
- u64 ac7_max;
- u64 ac8_max;
- u64 ac9_max;
+ struct_group(data,
+ u64 weight;
+ u64 ac0_max;
+ u64 ac1_max;
+ u64 ac2_max;
+ u64 ac3_max;
+ u64 ac4_max;
+ u64 ac5_max;
+ u64 ac6_max;
+ u64 ac7_max;
+ u64 ac8_max;
+ u64 ac9_max;
+ );
} __packed;

struct trt {
@@ -47,17 +49,19 @@ union art_object {
struct {
char source_device[8]; /* ACPI single name */
char target_device[8]; /* ACPI single name */
- u64 weight;
- u64 ac0_max_level;
- u64 ac1_max_level;
- u64 ac2_max_level;
- u64 ac3_max_level;
- u64 ac4_max_level;
- u64 ac5_max_level;
- u64 ac6_max_level;
- u64 ac7_max_level;
- u64 ac8_max_level;
- u64 ac9_max_level;
+ struct_group(data,
+ u64 weight;
+ u64 ac0_max_level;
+ u64 ac1_max_level;
+ u64 ac2_max_level;
+ u64 ac3_max_level;
+ u64 ac4_max_level;
+ u64 ac5_max_level;
+ u64 ac6_max_level;
+ u64 ac7_max_level;
+ u64 ac8_max_level;
+ u64 ac9_max_level;
+ );
};
u64 __data[ACPI_NR_ART_ELEMENTS];
};
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/infiniband/hw/cxgb4/cm.c | 5 +++--
drivers/net/ethernet/chelsio/cxgb4/t4_msg.h | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index 291471d12197..7129ae025b2d 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -2471,7 +2471,8 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff *skb,
skb_get(skb);
rpl = cplhdr(skb);
if (!is_t4(adapter_type)) {
- skb_trim(skb, roundup(sizeof(*rpl5), 16));
+ BUILD_BUG_ON(sizeof(*rpl5) != roundup(sizeof(*rpl5), 16));
+ skb_trim(skb, sizeof(*rpl5));
rpl5 = (void *)rpl;
INIT_TP_WR(rpl5, ep->hwtid);
} else {
@@ -2487,7 +2488,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff *skb,
opt2 |= CONG_CNTRL_V(CONG_ALG_TAHOE);
opt2 |= T5_ISS_F;
rpl5 = (void *)rpl;
- memset(&rpl5->iss, 0, roundup(sizeof(*rpl5)-sizeof(*rpl), 16));
+ memset_after(rpl5, 0, opt0);
if (peer2peer)
isn += 4;
rpl5->iss = cpu_to_be32(isn);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
index fed5f93bf620..26433a62d7f0 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
@@ -497,7 +497,7 @@ struct cpl_t5_pass_accept_rpl {
__be32 opt2;
__be64 opt0;
__be32 iss;
- __be32 rsvd;
+ __be32 rsvd[3];
};

struct cpl_act_open_req {
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index 585fb00bdce8..830066e95260 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -644,6 +644,7 @@ struct mlx5_ib_mr {
struct ib_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 {
bool is_odp_implicit;
};
};
+ );
};

/* Zero the fields in the mr that are variant depending on usage */
static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
{
- memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
+ memset(&mr->cleared, 0, sizeof(mr->cleared));
}

static inline bool is_odp_mr(struct mlx5_ib_mr *mr)
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
index 01ac1e93d27a..309dfe8c94fb 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_main.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
@@ -2419,7 +2419,7 @@ static int qede_load(struct qede_dev *edev, enum qede_load_mode mode,
goto out;
err4:
qede_sync_free_irqs(edev);
- memset(&edev->int_info.msix_cnt, 0, sizeof(struct qed_int_info));
+ memset_after(&edev->int_info, 0, msix);
err3:
qede_napi_disable_remove(edev);
err2:
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/iommu/amd/init.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 46280e6e1535..2df84737417b 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -121,8 +121,10 @@ struct ivhd_entry {
u8 type;
u16 devid;
u8 flags;
- u32 ext;
- u32 hidh;
+ struct_group(ext_hid,
+ u32 ext;
+ u32 hidh;
+ );
u64 cid;
u8 uidf;
u8 uidl;
@@ -1378,7 +1380,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu *iommu,
break;
}

- memcpy(hid, (u8 *)(&e->ext), ACPIHID_HID_LEN - 1);
+ BUILD_BUG_ON(sizeof(e->ext_hid) != ACPIHID_HID_LEN - 1);
+ memcpy(hid, &e->ext_hid, ACPIHID_HID_LEN - 1);
hid[ACPIHID_HID_LEN - 1] = '\0';

if (!(*hid)) {
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

net/dccp/trace.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/dccp/trace.h b/net/dccp/trace.h
index 5062421beee9..3c2594da49fc 100644
--- a/net/dccp/trace.h
+++ b/net/dccp/trace.h
@@ -60,9 +60,7 @@ TRACE_EVENT(dccp_probe,
__entry->tx_t_ipi = hc->tx_t_ipi;
} else {
__entry->tx_s = 0;
- memset(&__entry->tx_rtt, 0, (void *)&__entry->tx_t_ipi -
- (void *)&__entry->tx_rtt +
- sizeof(__entry->tx_t_ipi));
+ memset_after(__entry, 0, tx_s);
}
),

--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/net/wireless/ath/ath11k/hal_rx.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/hal_rx.c b/drivers/net/wireless/ath/ath11k/hal_rx.c
index 325055ca41ab..c72b6b45b3ba 100644
--- a/drivers/net/wireless/ath/ath11k/hal_rx.c
+++ b/drivers/net/wireless/ath/ath11k/hal_rx.c
@@ -29,8 +29,7 @@ static int ath11k_hal_reo_cmd_queue_stats(struct hal_tlv_hdr *tlv,
FIELD_PREP(HAL_TLV_HDR_LEN, sizeof(*desc));

desc = (struct hal_reo_get_queue_stats *)tlv->value;
- memset(&desc->queue_addr_lo, 0,
- (sizeof(*desc) - sizeof(struct hal_reo_cmd_hdr)));
+ memset_after(desc, 0, cmd);

desc->cmd.info0 &= ~HAL_REO_CMD_HDR_INFO0_STATUS_REQUIRED;
if (cmd->flag & HAL_REO_CMD_FLG_NEED_STATUS)
@@ -62,8 +61,7 @@ static int ath11k_hal_reo_cmd_flush_cache(struct ath11k_hal *hal, struct hal_tlv
FIELD_PREP(HAL_TLV_HDR_LEN, sizeof(*desc));

desc = (struct hal_reo_flush_cache *)tlv->value;
- memset(&desc->cache_addr_lo, 0,
- (sizeof(*desc) - sizeof(struct hal_reo_cmd_hdr)));
+ memset_after(desc, 0, cmd);

desc->cmd.info0 &= ~HAL_REO_CMD_HDR_INFO0_STATUS_REQUIRED;
if (cmd->flag & HAL_REO_CMD_FLG_NEED_STATUS)
@@ -101,8 +99,7 @@ static int ath11k_hal_reo_cmd_update_rx_queue(struct hal_tlv_hdr *tlv,
FIELD_PREP(HAL_TLV_HDR_LEN, sizeof(*desc));

desc = (struct hal_reo_update_rx_queue *)tlv->value;
- memset(&desc->queue_addr_lo, 0,
- (sizeof(*desc) - sizeof(struct hal_reo_cmd_hdr)));
+ memset_after(desc, 0, cmd);

desc->cmd.info0 &= ~HAL_REO_CMD_HDR_INFO0_STATUS_REQUIRED;
if (cmd->flag & HAL_REO_CMD_FLG_NEED_STATUS)
@@ -762,15 +759,17 @@ void ath11k_hal_reo_qdesc_setup(void *vaddr, int tid, u32 ba_window_size,
* size changes and also send WMI message to FW to change the REO
* queue descriptor in Rx peer entry as part of dp_rx_tid_update.
*/
- memset(ext_desc, 0, 3 * sizeof(*ext_desc));
+ memset(ext_desc, 0, sizeof(*ext_desc));
ath11k_hal_reo_set_desc_hdr(&ext_desc->desc_hdr, HAL_DESC_REO_OWNED,
HAL_DESC_REO_QUEUE_EXT_DESC,
REO_QUEUE_DESC_MAGIC_DEBUG_PATTERN_1);
ext_desc++;
+ memset(ext_desc, 0, sizeof(*ext_desc));
ath11k_hal_reo_set_desc_hdr(&ext_desc->desc_hdr, HAL_DESC_REO_OWNED,
HAL_DESC_REO_QUEUE_EXT_DESC,
REO_QUEUE_DESC_MAGIC_DEBUG_PATTERN_2);
ext_desc++;
+ memset(ext_desc, 0, sizeof(*ext_desc));
ath11k_hal_reo_set_desc_hdr(&ext_desc->desc_hdr, HAL_DESC_REO_OWNED,
HAL_DESC_REO_QUEUE_EXT_DESC,
REO_QUEUE_DESC_MAGIC_DEBUG_PATTERN_3);
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/net/wireless/intel/iwlwifi/fw/file.h b/drivers/net/wireless/intel/iwlwifi/fw/file.h
index 9a8c7b7a0816..226ccd3a6612 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/file.h
+++ b/drivers/net/wireless/intel/iwlwifi/fw/file.h
@@ -116,7 +116,7 @@ enum iwl_ucode_tlv_type {
struct iwl_ucode_tlv {
__le32 type; /* see above */
__le32 length; /* not including type/length fields */
- u8 data[0];
+ u8 data[];
};

#define IWL_TLV_UCODE_MAGIC 0x0a4c5749
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
index 0ddd255a8cc1..f4efddf3e3c3 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
@@ -71,7 +71,8 @@ static int iwl_dbg_tlv_add(const struct iwl_ucode_tlv *tlv,
if (!node)
return -ENOMEM;

- memcpy(&node->tlv, tlv, sizeof(node->tlv) + len);
+ memcpy(&node->tlv, tlv, sizeof(node->tlv));
+ memcpy(node->tlv.data, tlv->data, len);
list_add_tail(&node->list, list);

return 0;
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

net/xfrm/xfrm_policy.c | 4 +---
net/xfrm/xfrm_user.c | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 37d17a79617c..1a06585022ab 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2486,9 +2486,7 @@ static inline struct xfrm_dst *xfrm_alloc_dst(struct net *net, int family)
xdst = dst_alloc(dst_ops, NULL, 1, DST_OBSOLETE_NONE, 0);

if (likely(xdst)) {
- struct dst_entry *dst = &xdst->u.dst;
-
- memset(dst + 1, 0, sizeof(*xdst) - sizeof(*dst));
+ memset_after(xdst, 0, u.dst);
} else
xdst = ERR_PTR(-ENOBUFS);

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index acc3a0dab331..0bf8fec3fd97 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2907,7 +2907,7 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct
copy_to_user_state(x, &ue->state);
ue->hard = (c->data.hard != 0) ? 1 : 0;
/* clear the padding bytes */
- memset(&ue->hard + 1, 0, sizeof(*ue) - offsetofend(typeof(*ue), hard));
+ memset_after(ue, 0, hard);

err = xfrm_mark_put(skb, &x->mark);
if (err)
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/hid/hid-roccat-kone.c | 2 +-
drivers/hid/hid-roccat-kone.h | 12 +++++++-----
2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 1ca64481145e..ea17abc7ad52 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -857,7 +857,7 @@ static int kone_raw_event(struct hid_device *hdev, struct hid_report *report,
memcpy(&kone->last_mouse_event, event,
sizeof(struct kone_mouse_event));
else
- memset(&event->tilt, 0, 5);
+ memset(&event->wipe, 0, sizeof(event->wipe));

kone_keep_values_up_to_date(kone, event);

diff --git a/drivers/hid/hid-roccat-kone.h b/drivers/hid/hid-roccat-kone.h
index 4a1a9cb76b08..65c800e3addc 100644
--- a/drivers/hid/hid-roccat-kone.h
+++ b/drivers/hid/hid-roccat-kone.h
@@ -152,11 +152,13 @@ struct kone_mouse_event {
uint16_t x;
uint16_t y;
uint8_t wheel; /* up = 1, down = -1 */
- uint8_t tilt; /* right = 1, left = -1 */
- uint8_t unknown;
- uint8_t event;
- uint8_t value; /* press = 0, release = 1 */
- uint8_t macro_key; /* 0 to 8 */
+ struct_group(wipe,
+ uint8_t tilt; /* right = 1, left = -1 */
+ uint8_t unknown;
+ uint8_t event;
+ uint8_t value; /* press = 0, release = 1 */
+ uint8_t macro_key; /* 0 to 8 */
+ );
} __attribute__ ((__packed__));

enum kone_mouse_events {
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
index 88444fe6d1c6..6d2115639434 100644
--- a/drivers/net/wireless/ath/carl9170/tx.c
+++ b/drivers/net/wireless/ath/carl9170/tx.c
@@ -278,9 +278,7 @@ static void carl9170_tx_release(struct kref *ref)
BUILD_BUG_ON(
offsetof(struct ieee80211_tx_info, status.ack_signal) != 20);

- memset(&txinfo->status.ack_signal, 0,
- sizeof(struct ieee80211_tx_info) -
- offsetof(struct ieee80211_tx_info, status.ack_signal));
+ memset_after(&txinfo->status, 0, rates);

if (atomic_read(&ar->tx_total_queued))
ar->tx_schedule = true;
diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c
index 873fea59894f..f71b355f8583 100644
--- a/drivers/net/wireless/intersil/p54/txrx.c
+++ b/drivers/net/wireless/intersil/p54/txrx.c
@@ -431,9 +431,7 @@ static void p54_rx_frame_sent(struct p54_common *priv, struct sk_buff *skb)
* Clear manually, ieee80211_tx_info_clear_status would
* clear the counts too and we need them.
*/
- memset(&info->status.ack_signal, 0,
- sizeof(struct ieee80211_tx_info) -
- offsetof(struct ieee80211_tx_info, status.ack_signal));
+ memset_after(&info->status, 0, rates);
BUILD_BUG_ON(offsetof(struct ieee80211_tx_info,
status.ack_signal) != 20);

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d8a1d09a2141..7abc1427aa8c 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1200,9 +1200,7 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)

BUILD_BUG_ON(
offsetof(struct ieee80211_tx_info, status.ack_signal) != 20);
- memset(&info->status.ampdu_ack_len, 0,
- sizeof(struct ieee80211_tx_info) -
- offsetof(struct ieee80211_tx_info, status.ampdu_ack_len));
+ memset_after(&info->status, 0, ack_signal);
}


--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

include/net/ip6_fib.h | 30 ++++++++++++++++--------------
net/ipv6/route.c | 4 +---
2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 15b7fbe6b15c..9816e7444918 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -205,20 +205,22 @@ struct fib6_info {

struct rt6_info {
struct dst_entry dst;
- struct fib6_info __rcu *from;
- int sernum;
-
- struct rt6key rt6i_dst;
- struct rt6key rt6i_src;
- struct in6_addr rt6i_gateway;
- struct inet6_dev *rt6i_idev;
- u32 rt6i_flags;
-
- struct list_head rt6i_uncached;
- struct uncached_list *rt6i_uncached_list;
-
- /* more non-fragment space at head required */
- unsigned short rt6i_nfheader_len;
+ struct_group(init,
+ struct fib6_info __rcu *from;
+ int sernum;
+
+ struct rt6key rt6i_dst;
+ struct rt6key rt6i_src;
+ struct in6_addr rt6i_gateway;
+ struct inet6_dev *rt6i_idev;
+ u32 rt6i_flags;
+
+ struct list_head rt6i_uncached;
+ struct uncached_list *rt6i_uncached_list;
+
+ /* more non-fragment space at head required */
+ unsigned short rt6i_nfheader_len;
+ );
};

struct fib6_result {
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 6b8051106aba..bbcc605bab57 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -327,9 +327,7 @@ static const struct rt6_info ip6_blk_hole_entry_template = {

static void rt6_info_init(struct rt6_info *rt)
{
- struct dst_entry *dst = &rt->dst;
-
- memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
+ memset(&rt->init, 0, sizeof(rt->init));
INIT_LIST_HEAD(&rt->rt6i_uncached);
}

--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

arch/x86/kvm/emulate.c | 3 +--
arch/x86/kvm/kvm_emulate.h | 19 +++++++++++--------
2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 2837110e66ed..2608a047e769 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5377,8 +5377,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop)

void init_decode_cache(struct x86_emulate_ctxt *ctxt)
{
- memset(&ctxt->rip_relative, 0,
- (void *)&ctxt->modrm - (void *)&ctxt->rip_relative);
+ memset(&ctxt->decode_cache, 0, sizeof(ctxt->decode_cache));

ctxt->io_read.pos = 0;
ctxt->io_read.end = 0;
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 68b420289d7e..9b8afcb8ad39 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -341,14 +341,17 @@ struct x86_emulate_ctxt {
* the rest are initialized unconditionally in x86_decode_insn
* or elsewhere
*/
- bool rip_relative;
- u8 rex_prefix;
- u8 lock_prefix;
- u8 rep_prefix;
- /* bitmaps of registers in _regs[] that can be read */
- u32 regs_valid;
- /* bitmaps of registers in _regs[] that have been written */
- u32 regs_dirty;
+ struct_group(decode_cache,
+ bool rip_relative;
+ u8 rex_prefix;
+ u8 lock_prefix;
+ u8 rep_prefix;
+ /* bitmaps of registers in _regs[] that can be read */
+ u32 regs_valid;
+ /* bitmaps of registers in _regs[] that have been written */
+ u32 regs_dirty;
+ );
+
/* modrm */
u8 modrm;
u8 modrm_mod;
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

net/ethtool/stats.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index ec07f5765e03..a20e0a24ff61 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -14,10 +14,12 @@ struct stats_req_info {

struct stats_reply_data {
struct ethnl_reply_data base;
- struct ethtool_eth_phy_stats phy_stats;
- struct ethtool_eth_mac_stats mac_stats;
- struct ethtool_eth_ctrl_stats ctrl_stats;
- struct ethtool_rmon_stats rmon_stats;
+ struct_group(stats,
+ struct ethtool_eth_phy_stats phy_stats;
+ struct ethtool_eth_mac_stats mac_stats;
+ struct ethtool_eth_ctrl_stats ctrl_stats;
+ struct ethtool_rmon_stats rmon_stats;
+ );
const struct ethtool_rmon_hist_range *rmon_ranges;
};

@@ -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
* from being reported to user space in case driver did not set them.
*/
- 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)
--
2.30.2

[Kees Cook]

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 <gusta...@kernel.org>
Signed-off-by: Gustavo A. R. Silva <gusta...@kernel.org>

Signed-off-by: Kees Cook <kees...@chromium.org>
---

Makefile | 1 -
1 file changed, 1 deletion(-)

diff --git a/Makefile b/Makefile
index 6f781a199624..77d01ba3d4e1 100644
--- a/Makefile
+++ b/Makefile
@@ -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 least
KBUILD_CFLAGS += $(call cc-disable-warning, zero-length-bounds)
-KBUILD_CFLAGS += $(call cc-disable-warning, array-bounds)
KBUILD_CFLAGS += $(call cc-disable-warning, stringop-overflow)

# Another good warning that we'll want to enable eventually
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

include/net/netfilter/nf_conntrack.h | 20 ++++++++++----------
net/netfilter/nf_conntrack_core.c | 4 +---
2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index cc663c68ddc4..51004fce2937 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -101,24 +101,24 @@ struct nf_conn {
struct hlist_node nat_bysource;
#endif
/* all members below initialized via memset */
- struct { } __nfct_init_offset;
-
- /* If we were expected by an expectation, this will be it */
- struct nf_conn *master;
+ struct_group(init,
+ /* If we were expected by an expectation, this will be it */
+ struct nf_conn *master;

#if defined(CONFIG_NF_CONNTRACK_MARK)
- u_int32_t mark;
+ u_int32_t mark;
#endif

#ifdef CONFIG_NF_CONNTRACK_SECMARK
- u_int32_t secmark;
+ u_int32_t secmark;
#endif

- /* Extensions */
- struct nf_ct_ext *ext;
+ /* Extensions */
+ struct nf_ct_ext *ext;

- /* Storage reserved for other modules, must be the last member */
- union nf_conntrack_proto proto;
+ /* Storage reserved for other modules, must be the last member */
+ union nf_conntrack_proto proto;
+ );
};

static inline struct nf_conn *
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 5c03e5106751..b05eb64ec788 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1527,9 +1527,7 @@ __nf_conntrack_alloc(struct net *net,
ct->status = 0;
ct->timeout = 0;
write_pnet(&ct->ct_net, net);
- memset(&ct->__nfct_init_offset, 0,
- offsetof(struct nf_conn, proto) -
- offsetof(struct nf_conn, __nfct_init_offset));
+ memset(&ct->init, 0, sizeof(ct->init));

nf_ct_zone_add(ct, zone);

--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

include/linux/string.h | 12 ++++++++++++
lib/test_memcpy.c | 12 ++++++++++++
2 files changed, 24 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index cbe889e404e2..4f9f67505f70 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -272,6 +272,18 @@ static __always_inline void memcpy_and_pad(void *dest, size_t dest_len,
memcpy(dest, src, dest_len);
}

+/**
+ * memset_after - Set a value after a struct member to the end of a struct
+ *
+ * @obj: Address of target struct instance
+ * @v: Byte value to repeatedly write
+ * @member: after which struct member to start writing bytes
+ */
+#define memset_after(obj, v, member) do { \
+ memset((u8 *)(obj) + offsetofend(typeof(*(obj)), member), v, \
+ sizeof(*(obj)) - offsetofend(typeof(*(obj)), member)); \
+} while (0)
+
/**
* str_has_prefix - Test if a string has a given prefix
* @str: The string to test
diff --git a/lib/test_memcpy.c b/lib/test_memcpy.c
index 7c64120a68a9..f52b284f4410 100644
--- a/lib/test_memcpy.c
+++ b/lib/test_memcpy.c
@@ -223,6 +223,13 @@ static int __init test_memset(void)
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
},
};
+ struct some_bytes after = {
+ .data = { 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x72,
+ 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72,
+ 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72,
+ 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72,
+ },
+ };
struct some_bytes dest = { };
int count, value;
u8 *ptr;
@@ -254,6 +261,11 @@ static int __init test_memset(void)
memset(ptr++, value++, count++);
compare("argument side-effects", dest, three);

+ /* Verify memset_after() */
+ dest = control;
+ memset_after(&dest, 0x72, three);
+ compare("memset_after()", dest, after);
+
return 0;
#undef TEST_OP
}
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index e6a3eaaa57d9..7e8beb42d2d3 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd,
return SCSI_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);
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

include/uapi/linux/netlink.h | 1 +
net/netlink/af_netlink.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 4c0cde075c27..ddeaa748df5e 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -47,6 +47,7 @@ struct nlmsghdr {
__u16 nlmsg_flags; /* Additional flags */
__u32 nlmsg_seq; /* Sequence number */
__u32 nlmsg_pid; /* Sending process port ID */
+ __u8 contents[];
};

/* Flags values */
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 24b7cf447bc5..f2dd99e96822 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2447,7 +2447,9 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
NLMSG_ERROR, payload, flags);
errmsg = nlmsg_data(rep);
errmsg->error = err;
- memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh));
+ memcpy(&errmsg->msg, nlh, sizeof(*nlh));
+ if (payload > sizeof(*errmsg))
+ memcpy(errmsg->msg.contents, nlh->contents, nlh->nlmsg_len - sizeof(*nlh));

if (nlk_has_extack && extack) {
if (extack->_msg) {
--
2.30.2

[Kees Cook]

As done for memcpy(), also update memmove() to use the same tightened
compile-time checks under CONFIG_FORTIFY_SOURCE.

Signed-off-by: Kees Cook <kees...@chromium.org>
---

arch/x86/boot/compressed/misc.c | 3 ++-
arch/x86/lib/memcpy_32.c | 1 +
include/linux/fortify-string.h | 21 ++++---------------
.../read_overflow2_field-memmove.c | 5 +++++
.../write_overflow_field-memmove.c | 5 +++++
5 files changed, 17 insertions(+), 18 deletions(-)
create mode 100644 lib/test_fortify/read_overflow2_field-memmove.c
create mode 100644 lib/test_fortify/write_overflow_field-memmove.c

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 743f13ea25c1..83ff4354970e 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -34,10 +34,11 @@
* try to define their own functions if these are not defined as macros.
*/
#define memzero(s, n) memset((s), 0, (n))
+#ifndef memmove
#define memmove memmove
-
/* Functions used by the included decompressor code below. */
void *memmove(void *dest, const void *src, size_t n);
+#endif

/*
* This is set up by the setup-routine at boot-time
diff --git a/arch/x86/lib/memcpy_32.c b/arch/x86/lib/memcpy_32.c
index e565d1c9019e..f19b7fd07f04 100644
--- a/arch/x86/lib/memcpy_32.c
+++ b/arch/x86/lib/memcpy_32.c
@@ -4,6 +4,7 @@

#undef memcpy
#undef memset
+#undef memmove

__visible void *memcpy(void *to, const void *from, size_t n)
{
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 5e79e626172b..2ffa5224aaac 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -284,22 +284,10 @@ __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
__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);
- size_t q_size = __builtin_object_size(q, 0);
-
- if (__builtin_constant_p(size)) {
- if (p_size < size)
- __write_overflow();
- if (q_size < size)
- __read_overflow2();
- }
- if (p_size < size || q_size < size)
- fortify_panic(__func__);
- return __underlying_memmove(p, q, size);
-}
+#define memmove(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), \
+ memmove)

extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan);
__FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size)
@@ -385,7 +373,6 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
/* Don't use these outside the FORITFY_SOURCE implementation */
#undef __underlying_memchr
#undef __underlying_memcmp
-#undef __underlying_memmove
#undef __underlying_memset
#undef __underlying_strcat
#undef __underlying_strcpy
diff --git a/lib/test_fortify/read_overflow2_field-memmove.c b/lib/test_fortify/read_overflow2_field-memmove.c
new file mode 100644
index 000000000000..6cc2724c8f62
--- /dev/null
+++ b/lib/test_fortify/read_overflow2_field-memmove.c

@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \

+ memmove(large, instance.buf, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow_field-memmove.c b/lib/test_fortify/write_overflow_field-memmove.c
new file mode 100644
index 000000000000..377fcf9bb2fd
--- /dev/null
+++ b/lib/test_fortify/write_overflow_field-memmove.c

@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \

+ memmove(instance.buf, large, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/net/wireless/intersil/hostap/hostap_hw.c b/drivers/net/wireless/intersil/hostap/hostap_hw.c
index 9a19046217df..cea8a9ddc4da 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_hw.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_hw.c
@@ -1812,8 +1812,9 @@ static int prism2_tx_80211(struct sk_buff *skb, struct net_device *dev)
memset(&txdesc, 0, sizeof(txdesc));

/* skb->data starts with txdesc->frame_control */
- hdr_len = 24;
- skb_copy_from_linear_data(skb, &txdesc.frame_control, hdr_len);
+ hdr_len = sizeof(txdesc.frame);
+ BUILD_BUG_ON(hdr_len != 24);
+ skb_copy_from_linear_data(skb, &txdesc.frame, hdr_len);
if (ieee80211_is_data(txdesc.frame_control) &&
ieee80211_has_a4(txdesc.frame_control) &&
skb->len >= 30) {
diff --git a/drivers/net/wireless/intersil/hostap/hostap_wlan.h b/drivers/net/wireless/intersil/hostap/hostap_wlan.h
index dd2603d9b5d3..174735a137c5 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_wlan.h
+++ b/drivers/net/wireless/intersil/hostap/hostap_wlan.h
@@ -115,12 +115,14 @@ struct hfa384x_tx_frame {
__le16 tx_control; /* HFA384X_TX_CTRL_ flags */

/* 802.11 */
- __le16 frame_control; /* parts not used */
- __le16 duration_id;
- u8 addr1[ETH_ALEN];
- u8 addr2[ETH_ALEN]; /* filled by firmware */
- u8 addr3[ETH_ALEN];
- __le16 seq_ctrl; /* filled by firmware */
+ struct_group(frame,
+ __le16 frame_control; /* parts not used */
+ __le16 duration_id;
+ u8 addr1[ETH_ALEN];
+ u8 addr2[ETH_ALEN]; /* filled by firmware */
+ u8 addr3[ETH_ALEN];
+ __le16 seq_ctrl; /* filled by firmware */
+ );
u8 addr4[ETH_ALEN];
__le16 data_len;

--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/hwtracing/intel_th/msu.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 432ade0842f6..f3e266b0756c 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -658,13 +658,11 @@ static void msc_buffer_clear_hw_header(struct msc *msc)

list_for_each_entry(win, &msc->win_list, entry) {
unsigned int blk;
- size_t hw_sz = sizeof(struct msc_block_desc) -
- offsetof(struct msc_block_desc, hw_tag);

for_each_sg(win->sgt->sgl, sg, win->nr_segs, blk) {
struct msc_block_desc *bdesc = sg_virt(sg);

- memset(&bdesc->hw_tag, 0, hw_sz);
+ memset_after(bdesc, 0, res0);
}
}
}
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index 6a099cb34b12..9080b2c5ffe8 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -1842,8 +1842,10 @@ static netdev_tx_t cxgb4_vf_eth_xmit(struct sk_buff *skb,
* (including the VLAN tag) into the header so we reject anything
* smaller than that ...
*/
- fw_hdr_copy_len = sizeof(wr->ethmacdst) + sizeof(wr->ethmacsrc) +
- sizeof(wr->ethtype) + sizeof(wr->vlantci);
+ BUILD_BUG_ON(sizeof(wr->firmware) !=
+ (sizeof(wr->ethmacdst) + sizeof(wr->ethmacsrc) +
+ sizeof(wr->ethtype) + sizeof(wr->vlantci)));
+ fw_hdr_copy_len = sizeof(wr->firmware);
ret = cxgb4_validate_skb(skb, dev, fw_hdr_copy_len);
if (ret)
goto out_free;
@@ -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
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h
index 0a326c054707..2419459a0b85 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h
@@ -794,10 +794,12 @@ struct fw_eth_tx_pkt_vm_wr {
__be32 op_immdlen;
__be32 equiq_to_len16;
__be32 r3[2];
- u8 ethmacdst[6];
- u8 ethmacsrc[6];
- __be16 ethtype;
- __be16 vlantci;
+ struct_group(firmware,
+ u8 ethmacdst[ETH_ALEN];
+ u8 ethmacsrc[ETH_ALEN];
+ __be16 ethtype;
+ __be16 vlantci;
+ );
};

#define FW_CMD_MAX_TIMEOUT 10000
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/sge.c b/drivers/net/ethernet/chelsio/cxgb4vf/sge.c
index 7bc80eeb2c21..671ca93e64ab 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/sge.c
@@ -1167,10 +1167,7 @@ netdev_tx_t t4vf_eth_xmit(struct sk_buff *skb, struct net_device *dev)
struct cpl_tx_pkt_core *cpl;
const struct skb_shared_info *ssi;
dma_addr_t addr[MAX_SKB_FRAGS + 1];
- const size_t fw_hdr_copy_len = (sizeof(wr->ethmacdst) +
- sizeof(wr->ethmacsrc) +
- sizeof(wr->ethtype) +
- sizeof(wr->vlantci));
+ const size_t fw_hdr_copy_len = sizeof(wr->firmware);

/*
* The chip minimum packet length is 10 octets but the firmware
@@ -1267,7 +1264,7 @@ netdev_tx_t t4vf_eth_xmit(struct sk_buff *skb, struct net_device *dev)
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;

/*
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

include/linux/fortify-string.h | 33 +++++++++++++++++++++------------
security/Kconfig | 2 +-
2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 718325331021..4afd42079d3b 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -38,10 +38,11 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
#define __underlying_strncpy __builtin_strncpy
#endif

-__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
+#define strncpy(p, q, s) __fortify_strncpy(p, q, s, __builtin_object_size(p, 1))
+__FORTIFY_INLINE char *__fortify_strncpy(char *p, const char *q,
+ __kernel_size_t size,
+ const size_t p_size)
{
- size_t p_size = __builtin_object_size(p, 1);
-
if (__builtin_constant_p(size) && p_size < size)
__write_overflow();
if (p_size < size)
@@ -112,12 +113,15 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)

/* defined after fortified strnlen to reuse it */
extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
-__FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t size)
+#define strscpy(p, q, s) __fortify_strscpy(p, q, s, \
+ __builtin_object_size(p, 1), \
+ __builtin_object_size(q, 1))
+__FORTIFY_INLINE ssize_t __fortify_strscpy(char *p, const char *q,
+ size_t size,
+ const size_t p_size,
+ const size_t q_size)
{
size_t len;
- /* Use string size rather than possible enclosing struct size. */
- size_t p_size = __builtin_object_size(p, 1);
- size_t q_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)
@@ -329,7 +333,8 @@ __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,

memmove)

extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan);

-__FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size)
+#define memscan(p, c, s) __fortify_memscan(p, c, s)
+__FORTIFY_INLINE void *__fortify_memscan(void *p, int c, __kernel_size_t size)
{

size_t p_size = __builtin_object_size(p, 0);

@@ -340,7 +345,8 @@ __FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size)
return __real_memscan(p, c, size);
}

-__FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)
+#define memcmp(p, q, s) __fortify_memcmp(p, q, s)
+__FORTIFY_INLINE int __fortify_memcmp(const void *p, const void *q, __kernel_size_t size)
{

size_t p_size = __builtin_object_size(p, 0);

size_t q_size = __builtin_object_size(q, 0);

@@ -356,7 +362,8 @@ __FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)
return __underlying_memcmp(p, q, size);
}

-__FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)
+#define memchr(p, c, s) __fortify_memchr(p, c, s)
+__FORTIFY_INLINE void *__fortify_memchr(const void *p, int c, __kernel_size_t size)
{

size_t p_size = __builtin_object_size(p, 0);

@@ -368,7 +375,8 @@ __FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)
}

void *__real_memchr_inv(const void *s, int c, size_t n) __RENAME(memchr_inv);
-__FORTIFY_INLINE void *memchr_inv(const void *p, int c, size_t size)
+#define memchr_inv(p, c, s) __fortify_memchr_inv(p, c, s)
+__FORTIFY_INLINE void *__fortify_memchr_inv(const void *p, int c, size_t size)
{

size_t p_size = __builtin_object_size(p, 0);

@@ -392,7 +400,8 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)
}

/* Defined after fortified strlen to reuse it. */
-__FORTIFY_INLINE char *strcpy(char *p, const char *q)
+#define strcpy(p, q) __fortify_strcpy(p, q)
+__FORTIFY_INLINE char *__fortify_strcpy(char *p, const char *q)
{
size_t p_size = __builtin_object_size(p, 1);
size_t q_size = __builtin_object_size(q, 1);
diff --git a/security/Kconfig b/security/Kconfig
index 8f0e675e70a4..509ec61bc54b 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -193,7 +193,7 @@ config FORTIFY_SOURCE
depends on ARCH_HAS_FORTIFY_SOURCE
# https://bugs.llvm.org/show_bug.cgi?id=50322
# https://bugs.llvm.org/show_bug.cgi?id=41459
- depends on !CONFIG_CC_IS_CLANG
+ depends on !CONFIG_CC_IS_CLANG || CLANG_VERSION >= 130000
help
Detect overflows of buffers in common string and memory functions
where the compiler can determine and validate the buffer sizes.
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/infiniband/hw/mthca/mthca_mr.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/mthca/mthca_mr.c b/drivers/infiniband/hw/mthca/mthca_mr.c
index ce0e0867e488..64adba5c067d 100644
--- a/drivers/infiniband/hw/mthca/mthca_mr.c
+++ b/drivers/infiniband/hw/mthca/mthca_mr.c
@@ -469,8 +469,7 @@ int mthca_mr_alloc(struct mthca_dev *dev, u32 pd, int buffer_size_shift,
mpt_entry->start = cpu_to_be64(iova);
mpt_entry->length = cpu_to_be64(total_size);

- memset(&mpt_entry->lkey, 0,
- sizeof *mpt_entry - offsetof(struct mthca_mpt_entry, lkey));
+ memset_after(mpt_entry, 0, length);

if (mr->mtt)
mpt_entry->mtt_seg =
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/char/pcmcia/cm4000_cs.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
index 8f1bce0b4fe5..2f43e7088e16 100644
--- a/drivers/char/pcmcia/cm4000_cs.c
+++ b/drivers/char/pcmcia/cm4000_cs.c
@@ -116,8 +116,9 @@ struct cm4000_dev {
wait_queue_head_t atrq; /* wait for ATR valid */
wait_queue_head_t readq; /* used by write to wake blk.read */

- /* warning: do not move this fields.
+ /* warning: do not move this struct group.
* initialising to zero depends on it - see ZERO_DEV below. */
+ struct_group(init,
unsigned char atr_csum;
unsigned char atr_len_retry;
unsigned short atr_len;
@@ -140,12 +141,10 @@ struct cm4000_dev {

struct timer_list timer; /* used to keep monitor running */
int monitor_running;
+ );
};

-#define ZERO_DEV(dev) \
- memset(&dev->atr_csum,0, \
- sizeof(struct cm4000_dev) - \
- offsetof(struct cm4000_dev, atr_csum))
+#define ZERO_DEV(dev) memset(&dev->init, 0, sizeof(dev->init))

static struct pcmcia_device *dev_table[CM4000_MAX_DEV];
static struct class *cmm_class;
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/staging/wlan-ng/hfa384x.h | 16 +++++++++-------
drivers/staging/wlan-ng/hfa384x_usb.c | 4 +++-
2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/wlan-ng/hfa384x.h b/drivers/staging/wlan-ng/hfa384x.h
index 88e894dd3568..87eb87e3beab 100644
--- a/drivers/staging/wlan-ng/hfa384x.h
+++ b/drivers/staging/wlan-ng/hfa384x.h
@@ -476,13 +476,15 @@ struct hfa384x_tx_frame {

/*-- 802.11 Header Information --*/

- u16 frame_control;
- u16 duration_id;
- u8 address1[6];
- u8 address2[6];
- u8 address3[6];
- u16 sequence_control;
- u8 address4[6];
+ struct_group(p80211,
+ u16 frame_control;
+ u16 duration_id;
+ u8 address1[6];
+ u8 address2[6];
+ u8 address3[6];
+ u16 sequence_control;
+ u8 address4[6];
+ );
__le16 data_len; /* little endian format */

/*-- 802.3 Header Information --*/
diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
index f2a0e16b0318..38aaae7a2d69 100644
--- a/drivers/staging/wlan-ng/hfa384x_usb.c
+++ b/drivers/staging/wlan-ng/hfa384x_usb.c
@@ -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(union p80211_hdr));
+ memcpy(&hw->txbuff.txfrm.desc.p80211, p80211_hdr,
sizeof(union p80211_hdr));

/* if we're using host WEP, increase size by IV+ICV */
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h b/drivers/gpu/drm/amd/include/atomfirmware.h
index 3811e58dd857..694dee9d2691 100644
--- a/drivers/gpu/drm/amd/include/atomfirmware.h
+++ b/drivers/gpu/drm/amd/include/atomfirmware.h
@@ -2081,6 +2081,7 @@ struct atom_smc_dpm_info_v4_5
{
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
// I2C Control
struct smudpm_i2c_controller_config_v2 I2cControllers[8];

@@ -2159,7 +2160,7 @@ struct atom_smc_dpm_info_v4_5
uint32_t MvddRatio; // This is used for MVDD Vid workaround. It has 16 fractional bits (Q16.16)

uint32_t BoardReserved[9];
-
+ );
};

struct atom_smc_dpm_info_v4_6
@@ -2168,6 +2169,7 @@ struct atom_smc_dpm_info_v4_6
// section: board parameters
uint32_t i2c_padding[3]; // old i2c control are moved to new area

+ struct_group(dpm_info,
uint16_t maxvoltagestepgfx; // in mv(q2) max voltage step that smu will request. multiple steps are taken if voltage change exceeds this value.
uint16_t maxvoltagestepsoc; // in mv(q2) max voltage step that smu will request. multiple steps are taken if voltage change exceeds this value.

@@ -2246,12 +2248,14 @@ struct atom_smc_dpm_info_v4_6

// reserved
uint32_t boardreserved[10];
+ );
};

struct atom_smc_dpm_info_v4_7
{
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
// I2C Control
struct smudpm_i2c_controller_config_v2 I2cControllers[8];

@@ -2348,6 +2352,7 @@ struct atom_smc_dpm_info_v4_7
uint8_t Padding8_Psi2;

uint32_t BoardReserved[5];
+ );
};

struct smudpm_i2c_controller_config_v3
@@ -2478,6 +2483,7 @@ struct atom_smc_dpm_info_v4_10
struct atom_common_table_header table_header;

// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
// Telemetry Settings
uint16_t GfxMaxCurrent; // in Amps
uint8_t GfxOffset; // in Amps
@@ -2524,6 +2530,7 @@ struct atom_smc_dpm_info_v4_10
uint16_t spare5;

uint32_t reserved[16];
+ );
};

/*
diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
index 43d43d6addc0..8093a98800c3 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
@@ -643,6 +643,7 @@ typedef struct {
// SECTION: BOARD PARAMETERS

// SVI2 Board Parameters
+ struct_group(v4_6,
uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value.
uint16_t MaxVoltageStepSoc; // 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_t BoardVoltageCoeffB; // decode by /1000

uint32_t BoardReserved[7];
+ );

// Padding for MMHUB - do not modify this
uint32_t MmHubPadding[8]; // SMU internal use
-
} PPTable_t;

typedef struct {
diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
index 04752ade1016..0b4e6e907e95 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
@@ -725,6 +725,7 @@ typedef struct {
uint32_t Reserved[8];

// SECTION: BOARD PARAMETERS
+ struct_group(v4,
// I2C Control
I2cControllerConfig_t I2cControllers[NUM_I2C_CONTROLLERS];

@@ -809,10 +810,10 @@ typedef struct {
uint8_t Padding8_Loadline;

uint32_t BoardReserved[8];
+ );

// Padding for MMHUB - do not modify this
uint32_t MmHubPadding[8]; // SMU internal use
-
} PPTable_t;

typedef struct {
diff --git a/drivers/gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h b/drivers/gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h
index a017983ff1fa..5056d3728da8 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h
@@ -390,6 +390,7 @@ typedef struct {
uint32_t spare3[14];

// SECTION: BOARD PARAMETERS
+ struct_group(v4_10,
// Telemetry Settings
uint16_t GfxMaxCurrent; // in Amps
int8_t GfxOffset; // in Amps
@@ -444,7 +445,7 @@ typedef struct {

//reserved
uint32_t reserved[14];
-
+ );
} PPTable_t;

typedef struct {
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
index 6ec8492f71f5..19951399cb33 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
@@ -463,11 +463,11 @@ static int arcturus_append_powerplay_table(struct smu_context *smu)
smc_dpm_table->table_header.format_revision,
smc_dpm_table->table_header.content_revision);

+ BUILD_BUG_ON(sizeof(smc_pptable->v4_6) != sizeof(smc_dpm_table->dpm_info));
if ((smc_dpm_table->table_header.format_revision == 4) &&
(smc_dpm_table->table_header.content_revision == 6))
- memcpy(&smc_pptable->MaxVoltageStepGfx,
- &smc_dpm_table->maxvoltagestepgfx,
- sizeof(*smc_dpm_table) - offsetof(struct atom_smc_dpm_info_v4_6, maxvoltagestepgfx));
+ memcpy(&smc_pptable->v4_6, &smc_dpm_table->dpm_info,
+ sizeof(smc_dpm_table->dpm_info));

return 0;
}
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
index 59ea59acfb00..cb6665fbe319 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
@@ -431,16 +431,20 @@ static int navi10_append_powerplay_table(struct smu_context *smu)

switch (smc_dpm_table->table_header.content_revision) {
case 5: /* nv10 and nv14 */
- memcpy(smc_pptable->I2cControllers, smc_dpm_table->I2cControllers,
- sizeof(*smc_dpm_table) - sizeof(smc_dpm_table->table_header));
+ BUILD_BUG_ON(sizeof(smc_pptable->v4) !=
+ sizeof(smc_dpm_table->dpm_info));
+ memcpy(&smc_pptable->v4, &smc_dpm_table->dpm_info,
+ sizeof(smc_dpm_table->dpm_info));
break;
case 7: /* nv12 */
ret = amdgpu_atombios_get_data_table(adev, index, NULL, NULL, NULL,
(uint8_t **)&smc_dpm_table_v4_7);
if (ret)
return ret;
- memcpy(smc_pptable->I2cControllers, smc_dpm_table_v4_7->I2cControllers,
- sizeof(*smc_dpm_table_v4_7) - sizeof(smc_dpm_table_v4_7->table_header));
+ BUILD_BUG_ON(sizeof(smc_pptable->v4) !=
+ sizeof(smc_dpm_table_v4_7->dpm_info));
+ memcpy(&smc_pptable->v4, &smc_dpm_table_v4_7->dpm_info,
+ sizeof(smc_dpm_table_v4_7->dpm_info));
break;
default:
dev_err(smu->adev->dev, "smc_dpm_info with unsupported content revision %d!\n",
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
index 856eeaf293b8..c0645302fa50 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -407,11 +407,11 @@ static int aldebaran_append_powerplay_table(struct smu_context *smu)
smc_dpm_table->table_header.format_revision,
smc_dpm_table->table_header.content_revision);

+ BUILD_BUG_ON(sizeof(smc_pptable->v4_10) != sizeof(smc_dpm_table->dpm_info));
if ((smc_dpm_table->table_header.format_revision == 4) &&
(smc_dpm_table->table_header.content_revision == 10))
- memcpy(&smc_pptable->GfxMaxCurrent,
- &smc_dpm_table->GfxMaxCurrent,
- sizeof(*smc_dpm_table) - offsetof(struct atom_smc_dpm_info_v4_10, GfxMaxCurrent));
+ memcpy(&smc_pptable->v4_10, &smc_dpm_table->dpm_info,
+ sizeof(smc_dpm_table->dpm_info));
return 0;
}

--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

include/linux/trace_events.h | 26 ++++++++++++++------------
kernel/trace/trace.c | 4 +---
2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index ad413b382a3c..cadad77fe524 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -101,18 +101,20 @@ struct trace_iterator {
bool snapshot;

/* The below is zeroed out in pipe_read */
- struct trace_seq seq;
- struct trace_entry *ent;
- unsigned long lost_events;
- int leftover;
- int ent_size;
- int cpu;
- u64 ts;
-
- loff_t pos;
- long idx;
-
- /* All new field here will be zeroed out in pipe_read */
+ struct_group(init,
+ struct trace_seq seq;
+ struct trace_entry *ent;
+ unsigned long lost_events;
+ int leftover;
+ int ent_size;
+ int cpu;
+ u64 ts;
+
+ loff_t pos;
+ long idx;
+
+ /* All new field here will be zeroed out in pipe_read */
+ );
};

enum trace_iter_flags {
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c59dd35a6da5..9f83864b0be6 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6691,9 +6691,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
cnt = PAGE_SIZE - 1;

/* reset all but tr, trace, and overruns */
- memset(&iter->seq, 0,
- sizeof(struct trace_iterator) -
- offsetof(struct trace_iterator, seq));
+ memset(&iter->init, 0, sizeof(iter->init));
cpumask_clear(iter->started);
trace_seq_init(&iter->seq);
iter->pos = -1;
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/md/dm-integrity.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 40f8116c8e44..59deea0dd305 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -119,8 +119,10 @@ struct journal_entry {
#define JOURNAL_MAC_SIZE (JOURNAL_MAC_PER_SECTOR * JOURNAL_BLOCK_SECTORS)

struct journal_sector {
- __u8 entries[JOURNAL_SECTOR_DATA - JOURNAL_MAC_PER_SECTOR];
- __u8 mac[JOURNAL_MAC_PER_SECTOR];
+ struct_group(sectors,
+ __u8 entries[JOURNAL_SECTOR_DATA - JOURNAL_MAC_PER_SECTOR];
+ __u8 mac[JOURNAL_MAC_PER_SECTOR];
+ );
commit_id_t commit_id;
};

@@ -2856,7 +2858,8 @@ static void init_journal(struct dm_integrity_c *ic, unsigned start_section,
wraparound_section(ic, &i);
for (j = 0; j < ic->journal_section_sectors; j++) {
struct journal_sector *js = access_journal(ic, i, j);
- memset(&js->entries, 0, JOURNAL_SECTOR_DATA);
+ BUILD_BUG_ON(sizeof(js->sectors) != JOURNAL_SECTOR_DATA);
+ memset(&js->sectors, 0, sizeof(js->sectors));
js->commit_id = dm_integrity_commit_id(ic, i, j, commit_seq);
}
for (j = 0; j < ic->journal_section_entries; j++) {
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

drivers/macintosh/smu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/macintosh/smu.c b/drivers/macintosh/smu.c
index 94fb63a7b357..59ce431da7ef 100644
--- a/drivers/macintosh/smu.c
+++ b/drivers/macintosh/smu.c
@@ -848,7 +848,8 @@ int smu_queue_i2c(struct smu_i2c_cmd *cmd)
cmd->read = cmd->info.devaddr & 0x01;
switch(cmd->info.type) {
case SMU_I2C_TRANSFER_SIMPLE:
- memset(&cmd->info.sublen, 0, 4);
+ cmd->info.sublen = 0;
+ memset(&cmd->info.subaddr, 0, 3);
break;
case SMU_I2C_TRANSFER_COMBINED:
cmd->info.devaddr &= 0xfe;
--
2.30.2

[Kees Cook]

Before changing anything about memcpy(), memmove(), and memset(), add
run-time tests to check basic behaviors for any regressions.

Signed-off-by: Kees Cook <kees...@chromium.org>
---

lib/Kconfig.debug | 3 +
lib/Makefile | 1 +
lib/test_memcpy.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 289 insertions(+)
create mode 100644 lib/test_memcpy.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4654e838d68b..d315db9702de 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2220,6 +2220,9 @@ config TEST_XARRAY
config TEST_OVERFLOW
tristate "Test check_*_overflow() functions at runtime"

+config TEST_MEMCPY
+ tristate "Test memcpy*(), memmove*(), and memset*() functions at runtime"
+
config TEST_RHASHTABLE
tristate "Perform selftest on resizable hash table"
help
diff --git a/lib/Makefile b/lib/Makefile
index 40b4bf0bc847..083a19336e20 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_TEST_MIN_HEAP) += test_min_heap.o
obj-$(CONFIG_TEST_LKM) += test_module.o
obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
+obj-$(CONFIG_TEST_MEMCPY) += test_memcpy.o
obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
obj-$(CONFIG_TEST_SORT) += test_sort.o
obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
diff --git a/lib/test_memcpy.c b/lib/test_memcpy.c
new file mode 100644
index 000000000000..7c64120a68a9
--- /dev/null
+++ b/lib/test_memcpy.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for memcpy(), memmove(), and memset().
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/overflow.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/vmalloc.h>
+
+struct some_bytes {
+ union {
+ u8 data[32];
+ struct {
+ u32 one;
+ u16 two;
+ u8 three;
+ /* 1 byte hole */
+ u32 four[4];
+ };
+ };
+};
+
+#define check(instance, v) do { \
+ int i; \
+ BUILD_BUG_ON(sizeof(instance.data) != 32); \
+ for (i = 0; i < sizeof(instance.data); i++) { \
+ if (instance.data[i] != v) { \
+ pr_err("line %d: '%s' not initialized to 0x%02x @ %d (saw 0x%02x)\n", \
+ __LINE__, #instance, v, i, instance.data[i]); \
+ return 1; \
+ } \
+ } \
+} while (0)
+
+#define compare(name, one, two) do { \
+ int i; \
+ BUILD_BUG_ON(sizeof(one) != sizeof(two)); \
+ for (i = 0; i < sizeof(one); i++) { \
+ if (one.data[i] != two.data[i]) { \
+ pr_err("line %d: %s.data[%d] (0x%02x) != %s.data[%d] (0x%02x)\n", \
+ __LINE__, #one, i, one.data[i], \
+ #two, i, two.data[i]); \
+ return 1; \
+ } \
+ } \
+ pr_info("ok: " TEST_OP "() " name "\n"); \
+} while (0)
+
+static int __init test_memcpy(void)
+{
+#define TEST_OP "memcpy"
+ struct some_bytes control = {
+ .data = { 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ },
+ };
+ struct some_bytes zero = { };
+ struct some_bytes middle = {
+ .data = { 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ },
+ };
+ struct some_bytes three = {
+ .data = { 0x00, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x00, 0x00, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ },
+ };

+ struct some_bytes dest = { };

+ int count;
+ u8 *ptr;
+
+ /* Verify static initializers. */
+ check(control, 0x20);
+ check(zero, 0);
+ compare("static initializers", dest, zero);
+
+ /* Verify assignment. */
+ dest = control;
+ compare("direct assignment", dest, control);
+
+ /* Verify complete overwrite. */
+ memcpy(dest.data, zero.data, sizeof(dest.data));
+ compare("complete overwrite", dest, zero);
+
+ /* Verify middle overwrite. */
+ dest = control;
+ memcpy(dest.data + 12, zero.data, 7);
+ compare("middle overwrite", dest, middle);
+
+ /* Verify argument side-effects aren't repeated. */
+ dest = control;
+ ptr = dest.data;
+ count = 1;
+ memcpy(ptr++, zero.data, count++);
+ ptr += 8;
+ memcpy(ptr++, zero.data, count++);
+ compare("argument side-effects", dest, three);
+
+ return 0;
+#undef TEST_OP
+}
+
+static int __init test_memmove(void)
+{
+#define TEST_OP "memmove"
+ struct some_bytes control = {
+ .data = { 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };
+ struct some_bytes zero = { };
+ struct some_bytes middle = {
+ .data = { 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };
+ struct some_bytes five = {
+ .data = { 0x00, 0x00, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x00, 0x00, 0x00, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };
+ struct some_bytes overlap = {
+ .data = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+ 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };
+ struct some_bytes overlap_expected = {
+ .data = { 0x00, 0x01, 0x00, 0x01, 0x02, 0x03, 0x04, 0x07,
+ 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };

+ struct some_bytes dest = { };

+ int count;
+ u8 *ptr;
+
+ /* Verify static initializers. */
+ check(control, 0x99);
+ check(zero, 0);
+ compare("static initializers", zero, dest);
+
+ /* Verify assignment. */
+ dest = control;
+ compare("direct assignment", dest, control);
+
+ /* Verify complete overwrite. */
+ memmove(dest.data, zero.data, sizeof(dest.data));
+ compare("complete overwrite", dest, zero);
+
+ /* Verify middle overwrite. */
+ dest = control;
+ memmove(dest.data + 12, zero.data, 7);
+ compare("middle overwrite", dest, middle);
+
+ /* Verify argument side-effects aren't repeated. */
+ dest = control;
+ ptr = dest.data;
+ count = 2;
+ memmove(ptr++, zero.data, count++);
+ ptr += 9;
+ memmove(ptr++, zero.data, count++);
+ compare("argument side-effects", dest, five);
+
+ /* Verify overlapping overwrite is correct. */
+ ptr = &overlap.data[2];
+ memmove(ptr, overlap.data, 5);
+ compare("overlapping write", overlap, overlap_expected);
+
+ return 0;
+#undef TEST_OP
+}
+
+static int __init test_memset(void)
+{
+#define TEST_OP "memset"
+ struct some_bytes control = {
+ .data = { 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ },
+ };
+ struct some_bytes complete = {
+ .data = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ },
+ };
+ struct some_bytes middle = {
+ .data = { 0x30, 0x30, 0x30, 0x30, 0x31, 0x31, 0x31, 0x31,
+ 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31,
+ 0x31, 0x31, 0x31, 0x31, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ },
+ };
+ struct some_bytes three = {
+ .data = { 0x60, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x61, 0x61, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ },
+ };

+ struct some_bytes dest = { };

+ int count, value;
+ u8 *ptr;
+
+ /* Verify static initializers. */
+ check(control, 0x30);
+ check(dest, 0);
+
+ /* Verify assignment. */
+ dest = control;
+ compare("direct assignment", dest, control);
+
+ /* Verify complete overwrite. */
+ memset(dest.data, 0xff, sizeof(dest.data));
+ compare("complete overwrite", dest, complete);
+
+ /* Verify middle overwrite. */
+ dest = control;
+ memset(dest.data + 4, 0x31, 16);
+ compare("middle overwrite", dest, middle);
+
+ /* Verify argument side-effects aren't repeated. */
+ dest = control;
+ ptr = dest.data;
+ value = 0x60;
+ count = 1;
+ memset(ptr++, value++, count++);
+ ptr += 8;
+ memset(ptr++, value++, count++);
+ compare("argument side-effects", dest, three);
+
+ return 0;
+#undef TEST_OP
+}
+
+
+static int __init test_memcpy_init(void)
+{
+ int err = 0;
+
+ err |= test_memcpy();
+ err |= test_memmove();
+ err |= test_memset();
+
+ if (err) {
+ pr_warn("FAIL!\n");
+ err = -EINVAL;
+ } else {
+ pr_info("all tests passed\n");
+ }
+
+ return err;
+}
+
+static void __exit test_memcpy_exit(void)
+{ }
+
+module_init(test_memcpy_init);
+module_exit(test_memcpy_exit);
+MODULE_LICENSE("GPL");
--
2.30.2

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 7e67d02764db..5e79e626172b 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -2,13 +2,17 @@
#ifndef _LINUX_FORTIFY_STRING_H_
#define _LINUX_FORTIFY_STRING_H_

+#include <linux/bug.h>
+
#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); \
+})
+
+/*
+ * __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

diff --git a/include/linux/string.h b/include/linux/string.h
index 9473f81b9db2..cbe889e404e2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -261,8 +261,9 @@ static inline const char *kbasename(const char *path)
* @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)
{
if (dest_len > count) {
memcpy(dest, src, count);
diff --git a/lib/Makefile b/lib/Makefile
index 083a19336e20..74523fd394bd 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -370,7 +370,8 @@ TEST_FORTIFY_LOG = test_fortify.log
quiet_cmd_test_fortify = TEST $@
cmd_test_fortify = $(CONFIG_SHELL) $(srctree)/scripts/test_fortify.sh \
$< $@ "$(NM)" $(CC) $(c_flags) \
- $(call cc-disable-warning,fortify-source)
+ $(call cc-disable-warning,fortify-source) \
+ -DKBUILD_EXTRA_WARN1

targets += $(TEST_FORTIFY_LOGS)
clean-files += $(TEST_FORTIFY_LOGS)
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index faa9d8e4e2c5..4d205bf5993c 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -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);
+
void fortify_panic(const char *name)
{
pr_emerg("detected buffer overflow in %s\n", name);
diff --git a/lib/test_fortify/read_overflow2_field-memcpy.c b/lib/test_fortify/read_overflow2_field-memcpy.c
new file mode 100644
index 000000000000..de9569266223
--- /dev/null
+++ b/lib/test_fortify/read_overflow2_field-memcpy.c

@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \

+ memcpy(large, instance.buf, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow_field-memcpy.c b/lib/test_fortify/write_overflow_field-memcpy.c
new file mode 100644
index 000000000000..28cc81058dd3
--- /dev/null
+++ b/lib/test_fortify/write_overflow_field-memcpy.c

@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \

+ memcpy(instance.buf, large, sizeof(instance.buf) + 1)

[Nathan Chancellor]

On 7/27/2021 1:58 PM, Kees Cook wrote:
> 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 <kees...@chromium.org>
> ---

> security/Kconfig | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/security/Kconfig b/security/Kconfig
> index 0ced7fd33e4d..8f0e675e70a4 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -191,6 +191,9 @@ config HARDENED_USERCOPY_PAGESPAN
> config FORTIFY_SOURCE
> bool "Harden common str/mem functions against buffer overflows"
> depends on ARCH_HAS_FORTIFY_SOURCE
> + # https://bugs.llvm.org/show_bug.cgi?id=50322
> + # https://bugs.llvm.org/show_bug.cgi?id=41459
> + depends on !CONFIG_CC_IS_CLANG

Should be !CC_IS_CLANG, Kconfig is hard :)

> help
> Detect overflows of buffers in common string and memory functions
> where the compiler can determine and validate the buffer sizes.
>

Cheers,
Nathan

[Kees Cook]

As done for memcpy(), also update memset() to use the same tightened
compile-time bounds checking under CONFIG_FORTIFY_SOURCE.

Signed-off-by: Kees Cook <kees...@chromium.org>
---

include/linux/fortify-string.h | 54 ++++++++++++++++---
.../write_overflow_field-memset.c | 5 ++
2 files changed, 51 insertions(+), 8 deletions(-)
create mode 100644 lib/test_fortify/write_overflow_field-memset.c

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 2ffa5224aaac..718325331021 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -175,17 +175,56 @@ __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
return p;
}

-__FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size)
+__FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
+ const size_t p_size,
+ const size_t p_size_field)

{
- size_t p_size = __builtin_object_size(p, 0);

+ if (__builtin_constant_p(size)) {

+ /*
+ * Length argument is a constant expression, so we
+ * can perform compile-time bounds checking where
+ * buffer sizes are known.
+ */

- if (__builtin_constant_p(size) && p_size < size)

- __write_overflow();
- if (p_size < size)
- fortify_panic(__func__);
- return __underlying_memset(p, c, size);

+ /* Error when size is larger than enclosing struct. */
+ if (p_size > p_size_field && p_size < size)

+ __write_overflow();
+
+ /* Warn when write size is larger than dest field. */

+ if (p_size_field < size)
+ __write_overflow_field();
+ }

+ /*
+ * 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)
+ fortify_panic("memset");
}

+#define __fortify_memset_chk(p, c, size, p_size, p_size_field) ({ \

+ size_t __fortify_size = (size_t)(size); \

+ fortify_memset_chk(__fortify_size, p_size, p_size_field), \
+ __underlying_memset(p, c, __fortify_size); \

+})
+
+/*
+ * __builtin_object_size() must be captured here to avoid evaluating argument
+ * side-effects further into the macro layers.
+ */

+#define memset(p, c, s) __fortify_memset_chk(p, c, s, \
+ __builtin_object_size(p, 0), __builtin_object_size(p, 1))
+
/*

* To make sure the compiler can enforce protection against buffer overflows,

* memcpy(), memmove(), and memset() must not be used beyond individual

@@ -373,7 +412,6 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)

/* Don't use these outside the FORITFY_SOURCE implementation */
#undef __underlying_memchr
#undef __underlying_memcmp

-#undef __underlying_memset
#undef __underlying_strcat
#undef __underlying_strcpy
#undef __underlying_strlen
diff --git a/lib/test_fortify/write_overflow_field-memset.c b/lib/test_fortify/write_overflow_field-memset.c
new file mode 100644
index 000000000000..2331da26909e
--- /dev/null
+++ b/lib/test_fortify/write_overflow_field-memset.c

@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \

+ memset(instance.buf, 0x42, sizeof(instance.buf) + 1)

[Kees Cook]

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 <kees...@chromium.org>
---

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

diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 55234a558e98..b824679cfcb2 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -729,7 +729,8 @@ int drbd_send_sync_param(struct drbd_peer_device *peer_device)
cmd = apv >= 89 ? P_SYNC_PARAM89 : P_SYNC_PARAM;

/* initialize verify_alg and csums_alg */
- memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX);
+ BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX);
+ memset(&p->algs, 0, sizeof(p->algs));

if (get_ldev(peer_device->device)) {
dc = rcu_dereference(peer_device->device->ldev->disk_conf);
diff --git a/drivers/block/drbd/drbd_protocol.h b/drivers/block/drbd/drbd_protocol.h
index dea59c92ecc1..a882b65ab5d2 100644
--- a/drivers/block/drbd/drbd_protocol.h
+++ b/drivers/block/drbd/drbd_protocol.h
@@ -283,8 +283,10 @@ struct p_rs_param_89 {

struct p_rs_param_95 {
u32 resync_rate;
- char verify_alg[SHARED_SECRET_MAX];
- char csums_alg[SHARED_SECRET_MAX];
+ struct_group(algs,
+ char verify_alg[SHARED_SECRET_MAX];
+ char csums_alg[SHARED_SECRET_MAX];
+ );
u32 c_plan_ahead;
u32 c_delay_target;
u32 c_fill_target;
diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
index 1f740e42e457..6df2539e215b 100644
--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i

/* initialize verify_alg and csums_alg */
p = pi->data;
- memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX);
+ BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX);
+ memset(&p->algs, 0, sizeof(p->algs));

err = drbd_recv_all(peer_device->connection, p, header_size);
if (err)
--
2.30.2

[Kees Cook]

/me shakes fist at sky

Thank you! Fixed locally. :)

-Kees

>
> > help
> > Detect overflows of buffers in common string and memory functions
> > where the compiler can determine and validate the buffer sizes.
> >
>
> Cheers,
> Nathan

--
Kees Cook

[Larry Finger]

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<kees...@chromium.org>

Tested-by: Larry Finger <Larry....@lwfinger.net>
Acked-by: Larry Finger <Larry....@lwfinger.net>

Looks good.

Larry

[Nick Desaulniers]

On Tue, Jul 27, 2021 at 2:17 PM Kees Cook <kees...@chromium.org> wrote:
>
> To accelerate the review of potential run-time false positives, it's
> also worth noting that it is possible to partially automate checking
> by examining memcpy() buffer argument fields to see if they have
> a neighboring. It is reasonable to expect that the vast majority of

a neighboring...field?

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

What are you using from linux/bug.h here?

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__`?

Why __always_inline here?

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?

I haven't read the whole series yet, but I assume test_fortify.h was
provided earlier in the series?
--
Thanks,
~Nick Desaulniers

[Bart Van Assche]

Has it been considered to implement this test using the Kunit framework?

Thanks,

Bart.

[Randy Dunlap]

and do we want everything converted to use the Kunit test framework?

My answer is No, we don't, but I could easily be in the minority.

--
~Randy

[Michael Chan]

On Tue, Jul 27, 2021 at 2:01 PM Kees Cook <kees...@chromium.org> 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 <kees...@chromium.org>

Thanks.
Reviewed-by: Michael Chan <michae...@broadcom.com>

[Kees Cook]

Good point! I will see if that works here; it would make sense to make
this KUnit from the start.

--
Kees Cook

[Martin K. Petersen]

Kees,

> 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 <kees...@chromium.org>

Orthogonal to your change, it wasn't immediately obvious to me that
SRP_MAX_IU_LEN was the correct length to use for an srp_cmd. However, I
traversed the nested unions and it does look OK.

For good measure I copied Tyrel and Brian.

Acked-by: Martin K. Petersen <martin....@oracle.com>

> ---
> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index e6a3eaaa57d9..7e8beb42d2d3 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd,
> return SCSI_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);

--
Martin K. Petersen Oracle Linux Engineering

[Kees Cook]

On Tue, Jul 27, 2021 at 03:43:27PM -0700, Nick Desaulniers wrote:
> On Tue, Jul 27, 2021 at 2:17 PM Kees Cook <kees...@chromium.org> wrote:
> >
> > To accelerate the review of potential run-time false positives, it's
> > also worth noting that it is possible to partially automate checking
> > by examining memcpy() buffer argument fields to see if they have
> > a neighboring. It is reasonable to expect that the vast majority of
>
> a neighboring...field?

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

To accelerate the review of potential run-time false positives, it's
also worth noting that it is possible to partially automate checking

by examining the memcpy() buffer argument to check for the destination
struct member having a neighboring array member. It is reasonable to

expect that the vast majority of run-time false positives would look like
the already evaluated and fixed compile-time false positives, where the
most common pattern is neighboring arrays. (And, FWIW, several of the
compile-time fixes were actual bugs.)

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

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

> > [...]

> > +#define __fortify_memcpy_chk(p, q, size, p_size, q_size, \
> > + p_size_field, q_size_field, op) ({ \
> > + size_t __fortify_size = (size_t)(size); \
> > + fortify_memcpy_chk(__fortify_size, p_size, q_size, \
> > + p_size_field, q_size_field, #op); \
> > + __underlying_##op(p, q, __fortify_size); \
> > +})

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

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

> > [...]

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

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

> > [...]

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

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

> > [...]

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

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

-Kees

--
Kees Cook

[Gustavo A. R. Silva]

On Tue, Jul 27, 2021 at 01:57:56PM -0700, 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.
>

> 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 <kees...@chromium.org>

Reviewed-by: Gustavo A. R. Silva <gusta...@kernel.org>

Thanks
--
Gustavo

> ---
> drivers/net/wireguard/queueing.h | 4 +---
> include/linux/skbuff.h | 9 ++++-----
> net/core/skbuff.c | 14 +++++---------
> 3 files changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h
> index 4ef2944a68bc..52da5e963003 100644
> --- a/drivers/net/wireguard/queueing.h
> +++ b/drivers/net/wireguard/queueing.h
> @@ -79,9 +79,7 @@ static inline void wg_reset_packet(struct sk_buff *skb, bool encapsulating)
> u8 sw_hash = skb->sw_hash;
> u32 hash = skb->hash;
> skb_scrub_packet(skb, true);
> - memset(&skb->headers_start, 0,
> - offsetof(struct sk_buff, headers_end) -
> - offsetof(struct sk_buff, headers_start));
> + memset(&skb->headers, 0, sizeof(skb->headers));
> if (encapsulating) {
> skb->l4_hash = l4_hash;
> skb->sw_hash = sw_hash;
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index f19190820e63..b4032e9b130e 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -800,11 +800,10 @@ struct sk_buff {
> __u8 active_extensions;
> #endif
>
> - /* fields enclosed in headers_start/headers_end are copied
> + /* Fields enclosed in headers group are copied
> * using a single memcpy() in __copy_skb_header()
> */
> - /* private: */
> - __u32 headers_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 {
> u64 kcov_handle;
> #endif
>
> - /* private: */
> - __u32 headers_end[0];
> + ); /* end headers group */
> +
> /* public: */
>
> /* These elements must be at the end, see alloc_skb() for details. */
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index fc7942c0dddc..5f29c65507e0 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -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(struct sk_buff, field) < \
> - offsetof(struct sk_buff, headers_start)); \
> - BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
> - offsetof(struct sk_buff, headers_end)); \
> + BUILD_BUG_ON(offsetof(struct sk_buff, field) != \
> + offsetof(struct sk_buff, headers.field)); \
>
> static void __copy_skb_header(struct sk_buff *new, const struct sk_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.
> * It is not yet because we do not want to have a 16 bit hole
> */
> new->queue_mapping = old->queue_mapping;
>
> - memcpy(&new->headers_start, &old->headers_start,
> - offsetof(struct sk_buff, headers_end) -
> - offsetof(struct sk_buff, headers_start));
> + memcpy(&new->headers, &old->headers, sizeof(new->headers));
> CHECK_SKB_FIELD(protocol);
> CHECK_SKB_FIELD(csum);
> CHECK_SKB_FIELD(hash);
> --
> 2.30.2
>

[Gustavo A. R. Silva]

On Tue, Jul 27, 2021 at 01:57:57PM -0700, 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 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 <kees...@chromium.org>

Reviewed-by: Gustavo A. R. Silva <gusta...@kernel.org>

Thanks
--
Gustavo

> ---

> 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(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
> index 8a68df4d9e59..95c636f89329 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
> @@ -148,10 +148,10 @@ static int bnxt_hwrm_queue_cos2bw_qcfg(struct bnxt *bp, struct ieee_ets *ets)
> }
>
> data = &resp->queue_id0 + offsetof(struct bnxt_cos2bw_cfg, queue_id);
> - for (i = 0; i < bp->max_tc; i++, data += sizeof(cos2bw) - 4) {
> + for (i = 0; i < bp->max_tc; i++, data += sizeof(cos2bw.cfg)) {
> int tc;
>
> - memcpy(&cos2bw.queue_id, data, sizeof(cos2bw) - 4);
> + memcpy(&cos2bw.cfg, data, sizeof(cos2bw.cfg));
> if (i == 0)
> cos2bw.queue_id = resp->queue_id0;
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h
> index 6eed231de565..716742522161 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h
> @@ -23,13 +23,15 @@ struct bnxt_dcb {
>
> struct bnxt_cos2bw_cfg {
> u8 pad[3];
> - u8 queue_id;
> - __le32 min_bw;
> - __le32 max_bw;
> + struct_group_attr(cfg, __packed,
> + u8 queue_id;
> + __le32 min_bw;
> + __le32 max_bw;
> #define BW_VALUE_UNIT_PERCENT1_100 (0x1UL << 29)
> - u8 tsa;
> - u8 pri_lvl;
> - u8 bw_weight;
> + u8 tsa;
> + u8 pri_lvl;
> + u8 bw_weight;
> + );
> u8 unused;
> };
>
> --
> 2.30.2
>

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 01:57:58PM -0700, 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 <kees...@chromium.org>

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

Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 01:58:15PM -0700, 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() 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 <kees...@chromium.org>
> ---
> drivers/staging/wlan-ng/hfa384x.h | 16 +++++++++-------
> drivers/staging/wlan-ng/hfa384x_usb.c | 4 +++-
> 2 files changed, 12 insertions(+), 8 deletions(-)

Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 01:57:59PM -0700, 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

> 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 <kees...@chromium.org>

Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 01:58:00PM -0700, 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.
>

> 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 <kees...@chromium.org>
> ---

> drivers/staging/rtl8723bs/core/rtw_mlme.c | 2 +-

> 1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 01:58:40PM -0700, Kees Cook wrote:
> 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 <kees...@chromium.org>
> ---

> drivers/char/pcmcia/cm4000_cs.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)

Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 01:58:53PM -0700, 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.
>

> 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 <kees...@chromium.org>
> ---

> include/uapi/linux/netlink.h | 1 +
> net/netlink/af_netlink.c | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index 4c0cde075c27..ddeaa748df5e 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -47,6 +47,7 @@ struct nlmsghdr {
> __u16 nlmsg_flags; /* Additional flags */
> __u32 nlmsg_seq; /* Sequence number */
> __u32 nlmsg_pid; /* Sending process port ID */
> + __u8 contents[];

Is this ok to change a public, userspace visable, structure?

Nothing breaks?

thanks,

greg k-h

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 01:58:01PM -0700, 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

> 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 <kees...@chromium.org>
> ---

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

For the staging portion:

Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 01:58:10PM -0700, 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() 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 <kees...@chromium.org>
> ---

> 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(-)
>
> diff --git a/include/net/flow.h b/include/net/flow.h
> index 6f5e70240071..f1a3b6c8eae2 100644
> --- a/include/net/flow.h
> +++ b/include/net/flow.h
> @@ -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 */
> - __be32 saddr;
> - __be32 daddr;
> + struct_group(addrs,
> + __be32 saddr;
> + __be32 daddr;
> + );
>
> union flowi_uli uli;
> #define fl4_sport uli.ports.sport
> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
> index a0b637911d3c..8f5667b2ea92 100644
> --- a/include/uapi/linux/if_ether.h
> +++ b/include/uapi/linux/if_ether.h
> @@ -163,8 +163,16 @@
>
> #if __UAPI_DEF_ETHHDR
> struct ethhdr {
> - unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
> - unsigned char h_source[ETH_ALEN]; /* source ether addr */
> + union {
> + struct {
> + unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
> + unsigned char h_source[ETH_ALEN]; /* source ether addr */
> + };
> + struct {
> + unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
> + unsigned char h_source[ETH_ALEN]; /* source ether addr */
> + } addrs;

A union of the same fields in the same structure in the same way?

Ah, because struct_group() can not be used here? Still feels odd to see
in a userspace-visible header.

> + };
> __be16 h_proto; /* packet type ID field */
> } __attribute__((packed));
> #endif
> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
> index e42d13b55cf3..33647a37e56b 100644
> --- a/include/uapi/linux/ip.h
> +++ b/include/uapi/linux/ip.h
> @@ -100,8 +100,16 @@ struct iphdr {
> __u8 ttl;
> __u8 protocol;
> __sum16 check;
> - __be32 saddr;
> - __be32 daddr;
> + union {
> + struct {
> + __be32 saddr;
> + __be32 daddr;
> + } addrs;
> + struct {
> + __be32 saddr;
> + __be32 daddr;
> + };

Same here (except you named the first struct addrs, not the second,
unlike above).


> + };
> /*The options start here. */
> };
>
> diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
> index b243a53fa985..1c26d32e733b 100644
> --- a/include/uapi/linux/ipv6.h
> +++ b/include/uapi/linux/ipv6.h
> @@ -130,8 +130,16 @@ struct ipv6hdr {
> __u8 nexthdr;
> __u8 hop_limit;
>
> - struct in6_addr saddr;
> - struct in6_addr daddr;
> + union {
> + struct {
> + struct in6_addr saddr;
> + struct in6_addr daddr;
> + } addrs;
> + struct {
> + struct in6_addr saddr;
> + struct in6_addr daddr;
> + };

addrs first? Consistancy is key :)

thanks,

greg k-h

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 01:58:16PM -0700, 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() 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 <kees...@chromium.org>
> ---

> drivers/gpu/drm/mga/mga_ioc32.c | 30 ++++++++++++++------------
> include/uapi/drm/mga_drm.h | 37 ++++++++++++++++++++++++---------
> 2 files changed, 44 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/mga/mga_ioc32.c b/drivers/gpu/drm/mga/mga_ioc32.c
> index 4fd4de16cd32..fbd0329dbd4f 100644
> --- a/drivers/gpu/drm/mga/mga_ioc32.c
> +++ b/drivers/gpu/drm/mga/mga_ioc32.c
> @@ -38,16 +38,21 @@
> typedef struct drm32_mga_init {
> int func;
> u32 sarea_priv_offset;
> - int chipset;
> - int sgram;
> - unsigned int maccess;
> - unsigned int fb_cpp;
> - unsigned int front_offset, front_pitch;
> - unsigned int back_offset, back_pitch;
> - unsigned int depth_cpp;
> - unsigned int depth_offset, depth_pitch;
> - unsigned int texture_offset[MGA_NR_TEX_HEAPS];
> - unsigned int texture_size[MGA_NR_TEX_HEAPS];
> + struct_group(always32bit,
> + int chipset;
> + int sgram;
> + unsigned int maccess;
> + unsigned int fb_cpp;
> + unsigned int front_offset;
> + unsigned int front_pitch;
> + unsigned int back_offset;
> + unsigned int back_pitch;
> + unsigned int depth_cpp;
> + unsigned int depth_offset;
> + unsigned int depth_pitch;
> + unsigned int texture_offset[MGA_NR_TEX_HEAPS];
> + unsigned int texture_size[MGA_NR_TEX_HEAPS];
> + );
> u32 fb_offset;
> u32 mmio_offset;
> u32 status_offset;
> @@ -67,9 +72,8 @@ static int compat_mga_init(struct file *file, unsigned int cmd,
>
> init.func = init32.func;
> init.sarea_priv_offset = init32.sarea_priv_offset;
> - memcpy(&init.chipset, &init32.chipset,
> - offsetof(drm_mga_init_t, fb_offset) -
> - offsetof(drm_mga_init_t, chipset));
> + memcpy(&init.always32bit, &init32.always32bit,
> + sizeof(init32.always32bit));
> init.fb_offset = init32.fb_offset;
> init.mmio_offset = init32.mmio_offset;
> init.status_offset = init32.status_offset;
> diff --git a/include/uapi/drm/mga_drm.h b/include/uapi/drm/mga_drm.h
> index 8c4337548ab5..61612e5ecab2 100644
> --- a/include/uapi/drm/mga_drm.h
> +++ b/include/uapi/drm/mga_drm.h
> @@ -265,6 +265,16 @@ typedef struct _drm_mga_sarea {
> #define DRM_IOCTL_MGA_WAIT_FENCE DRM_IOWR(DRM_COMMAND_BASE + DRM_MGA_WAIT_FENCE, __u32)
> #define DRM_IOCTL_MGA_DMA_BOOTSTRAP DRM_IOWR(DRM_COMMAND_BASE + DRM_MGA_DMA_BOOTSTRAP, drm_mga_dma_bootstrap_t)
>
> +#define __struct_group(name, fields) \
> + union { \
> + struct { \
> + fields \
> + }; \
> + struct { \
> + fields \
> + } name; \
> + }
> +
> typedef struct _drm_mga_warp_index {
> int installed;
> unsigned long phys_addr;
> @@ -279,20 +289,25 @@ typedef struct drm_mga_init {
>
> unsigned long sarea_priv_offset;
>
> - int chipset;
> - int sgram;
> + __struct_group(always32bit,
> + int chipset;
> + int sgram;
>
> - unsigned int maccess;
> + unsigned int maccess;
>
> - unsigned int fb_cpp;
> - unsigned int front_offset, front_pitch;
> - unsigned int back_offset, back_pitch;
> + unsigned int fb_cpp;
> + unsigned int front_offset;
> + unsigned int front_pitch;
> + unsigned int back_offset;
> + unsigned int back_pitch;
>
> - unsigned int depth_cpp;
> - unsigned int depth_offset, depth_pitch;
> + unsigned int depth_cpp;
> + unsigned int depth_offset;
> + unsigned int depth_pitch;
>
> - unsigned int texture_offset[MGA_NR_TEX_HEAPS];
> - unsigned int texture_size[MGA_NR_TEX_HEAPS];
> + unsigned int texture_offset[MGA_NR_TEX_HEAPS];
> + unsigned int texture_size[MGA_NR_TEX_HEAPS];
> + );
>
> unsigned long fb_offset;
> unsigned long mmio_offset;
> @@ -302,6 +317,8 @@ typedef struct drm_mga_init {
> unsigned long buffers_offset;
> } drm_mga_init_t;
>
> +#undef __struct_group
> +

Why can you use __struct_group in this uapi header, but not the
networking one?

thanks,

greg k-h

[Gustavo A. R. Silva]

I think addrs should be second. In general, I think all newly added
non-anonymous structures should be second.

Thanks
--
Gustavo

[Greg Kroah-Hartman]

Why not use a local version of the macro like was done in the DRM header
file, to make it always work the same and more obvious what is
happening? If I were a userspace developer and saw the above, I would
think that the kernel developers have lost it :)

thanks,

greg k-h

[Gustavo A. R. Silva]

Then don't take a look at this[1]. :p

--
Gustavo

[1] https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d

[Gustavo A. R. Silva]

Yep; I agree. That one looks just fine. :)

--
Gustavo

[Greg Kroah-Hartman]

On Wed, Jul 28, 2021 at 01:31:16AM -0500, Gustavo A. R. Silva wrote:
> > Why not use a local version of the macro like was done in the DRM header
> > file, to make it always work the same and more obvious what is
> > happening? If I were a userspace developer and saw the above, I would
> > think that the kernel developers have lost it :)
>
> Then don't take a look at this[1]. :p
>
> --
> Gustavo
>
> [1] https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d

That one at least looks a "little" different so maybe it could be seen
as semi-reasonable :)

[David Sterba]

On Tue, Jul 27, 2021 at 01:58:38PM -0700, Kees Cook wrote:
> 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 <kees...@chromium.org>
> ---

> fs/btrfs/root-tree.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 702dc5441f03..ec9e78f65fca 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -39,10 +39,7 @@ static void btrfs_read_root_item(struct extent_buffer *eb, int slot,
> need_reset = 1;
> }
> if (need_reset) {
> - memset(&item->generation_v2, 0,
> - sizeof(*item) - offsetof(struct btrfs_root_item,
> - generation_v2));
> -

Please add
/* Clear all members from generation_v2 onwards */

> + memset_after(item, 0, level);
> generate_random_guid(item->uuid);

Acked-by: David Sterba <dst...@suse.com>

[Rasmus Villemoes]

On 27/07/2021 22.58, Kees Cook wrote:

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

I think the compiler would usually end up making that struct size 24,
with 4 bytes of trailing padding (at least when alignof(u64) is 8). In
that case, does __builtin_object_size(instance.array, 0) actually
evaluate to 18, or to 22? A quick test on x86-64 suggests the latter, so
the memcpy(, , 20) would not be a violation.

Perhaps it's better to base the example on something which doesn't have
potential trailing padding - so either add another 4 byte member, or
also make scalar2 u32.

Rasmus

[Rasmus Villemoes]

At least it should keep using a nlmsg_ prefix for consistency and reduce
risk of collision with somebody having defined an object-like contents
macro. But there's no guarantees in any case, of course.

Rasmus

[Stanislav Yakovlev]

On 28/07/2021, Kees Cook <kees...@chromium.org> wrote:
> 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 <kees...@chromium.org>
> ---

> drivers/net/wireless/intel/ipw2x00/libipw.h | 12 +++++++-----
> drivers/net/wireless/intel/ipw2x00/libipw_rx.c | 8 ++++----
> 2 files changed, 11 insertions(+), 9 deletions(-)
>

Acked-by: Stanislav Yakovlev <stas.y...@gmail.com>

Looks fine, thanks!

Stanislav.

[Kees Cook]

On Tue, Jul 27, 2021 at 09:39:39PM -0400, Martin K. Petersen wrote:
>
> Kees,
>
> > 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 <kees...@chromium.org>
>
> Orthogonal to your change, it wasn't immediately obvious to me that
> SRP_MAX_IU_LEN was the correct length to use for an srp_cmd. However, I
> traversed the nested unions and it does look OK.

Yeah, I had the same fun. Maybe I should add a BUILD_BUG_ON() here to
help illustrate the relationship? I did that in a few other places where
the equalities weren't very clear.

For example, change it to:

+ BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN);
+ memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp));

srp_cmd = &evt_struct->iu.srp.cmd;
- memset(srp_cmd, 0x00, SRP_MAX_IU_LEN);

>

> For good measure I copied Tyrel and Brian.
>
> Acked-by: Martin K. Petersen <martin....@oracle.com>

For the moment, I'll leave the patch as-is unless you prefer having
the BUILD_BUG_ON(). :)

Thanks!

-Kees

>
> > ---
> > drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> > index e6a3eaaa57d9..7e8beb42d2d3 100644
> > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> > @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd,
> > return SCSI_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);
>
> --
> Martin K. Petersen Oracle Linux Engineering

--
Kees Cook

[Kees Cook]

On Wed, Jul 28, 2021 at 07:55:53AM +0200, Greg Kroah-Hartman wrote:
> > struct ethhdr {
> > - unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
> > - unsigned char h_source[ETH_ALEN]; /* source ether addr */
> > + union {
> > + struct {
> > + unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
> > + unsigned char h_source[ETH_ALEN]; /* source ether addr */
> > + };
> > + struct {
> > + unsigned char h_dest[ETH_ALEN]; /* destination eth addr */
> > + unsigned char h_source[ETH_ALEN]; /* source ether addr */
> > + } addrs;
>
> A union of the same fields in the same structure in the same way?
>
> Ah, because struct_group() can not be used here? Still feels odd to see
> in a userspace-visible header.

Yeah, there is some inconsistency here. I will clean this up for v2.

Is there a place we can put kernel-specific macros for use in UAPI
headers? (I need to figure out where things like __kernel_size_t get
defined...)

--
Kees Cook

[Bart Van Assche]

On 7/27/21 1:58 PM, Kees Cook wrote:

> 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 <kees...@chromium.org>
> ---

> 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(-)
>
> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> index 55234a558e98..b824679cfcb2 100644
> --- a/drivers/block/drbd/drbd_main.c
> +++ b/drivers/block/drbd/drbd_main.c
> @@ -729,7 +729,8 @@ int drbd_send_sync_param(struct drbd_peer_device *peer_device)
> cmd = apv >= 89 ? P_SYNC_PARAM89 : P_SYNC_PARAM;
>
> /* initialize verify_alg and csums_alg */
> - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX);
> + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX);
> + memset(&p->algs, 0, sizeof(p->algs));
>
> if (get_ldev(peer_device->device)) {
> dc = rcu_dereference(peer_device->device->ldev->disk_conf);
> diff --git a/drivers/block/drbd/drbd_protocol.h b/drivers/block/drbd/drbd_protocol.h
> index dea59c92ecc1..a882b65ab5d2 100644
> --- a/drivers/block/drbd/drbd_protocol.h
> +++ b/drivers/block/drbd/drbd_protocol.h
> @@ -283,8 +283,10 @@ struct p_rs_param_89 {
>
> struct p_rs_param_95 {
> u32 resync_rate;
> - char verify_alg[SHARED_SECRET_MAX];
> - char csums_alg[SHARED_SECRET_MAX];
> + struct_group(algs,
> + char verify_alg[SHARED_SECRET_MAX];
> + char csums_alg[SHARED_SECRET_MAX];
> + );
> u32 c_plan_ahead;
> u32 c_delay_target;
> u32 c_fill_target;
> diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
> index 1f740e42e457..6df2539e215b 100644
> --- a/drivers/block/drbd/drbd_receiver.c
> +++ b/drivers/block/drbd/drbd_receiver.c
> @@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
>
> /* initialize verify_alg and csums_alg */
> p = pi->data;
> - memset(p->verify_alg, 0, 2 * SHARED_SECRET_MAX);
> + BUILD_BUG_ON(sizeof(p->algs) != 2 * SHARED_SECRET_MAX);
> + memset(&p->algs, 0, sizeof(p->algs));

Using struct_group() introduces complexity. Has it been considered not
to modify struct p_rs_param_95 and instead to use two memset() calls
instead of one (one memset() call per member)?

Thanks,

Bart.

[Kees Cook]

Perhaps there should be another helper memset_starting()? That would
make these cases a bit more self-documenting.

+ memset_starting(item, 0, generation_v2);

> > generate_random_guid(item->uuid);
>
> Acked-by: David Sterba <dst...@suse.com>

What do you think?

-Kees

--
Kees Cook

[Bart Van Assche]

How about using two memset() calls to clear h_dest[] and h_source[]
instead of modifying the uapi header?

Thanks,

Bart.

[Martin K. Petersen]

Kees,

> For example, change it to:
>
> + BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN);
> + memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp));
> srp_cmd = &evt_struct->iu.srp.cmd;
> - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN);

> For the moment, I'll leave the patch as-is unless you prefer having
> the BUILD_BUG_ON(). :)

I'm OK with the BUILD_BUG_ON(). Hopefully Tyrel or Brian will chime in.

[David Sterba]

That would be better, yes.

> + memset_starting(item, 0, generation_v2);

memset_from?

[Daniel Vetter]

If there's others, maybe we can stuff the uapi __struct_group into
linux/types.h where all the other __ uapi types hang out?

Anyway mga is very dead, I don't anyone cares.

Acked-by: Daniel Vetter <daniel...@ffwll.ch>

I'm assuming this goes in through a topic pull from you?

I'll leave the drm/amd one to figure out between you and Alex.
-Daniel

>
> thanks,
>
> greg k-h

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Jakub Kicinski]

On Tue, 27 Jul 2021 13:58:45 -0700 Kees Cook wrote:
> 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.

memset_after() ?

> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index 15b7fbe6b15c..9816e7444918 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -205,20 +205,22 @@ struct fib6_info {
>
> struct rt6_info {
> struct dst_entry dst;
> - struct fib6_info __rcu *from;
> - int sernum;
> -
> - struct rt6key rt6i_dst;
> - struct rt6key rt6i_src;
> - struct in6_addr rt6i_gateway;
> - struct inet6_dev *rt6i_idev;
> - u32 rt6i_flags;
> -
> - struct list_head rt6i_uncached;
> - struct uncached_list *rt6i_uncached_list;
> -
> - /* more non-fragment space at head required */
> - unsigned short rt6i_nfheader_len;
> + struct_group(init,
> + struct fib6_info __rcu *from;
> + int sernum;
> +
> + struct rt6key rt6i_dst;
> + struct rt6key rt6i_src;
> + struct in6_addr rt6i_gateway;
> + struct inet6_dev *rt6i_idev;
> + u32 rt6i_flags;
> +
> + struct list_head rt6i_uncached;
> + struct uncached_list *rt6i_uncached_list;
> +
> + /* more non-fragment space at head required */
> + unsigned short rt6i_nfheader_len;
> + );
> };
>
> struct fib6_result {
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 6b8051106aba..bbcc605bab57 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -327,9 +327,7 @@ static const struct rt6_info ip6_blk_hole_entry_template = {
>
> static void rt6_info_init(struct rt6_info *rt)
> {
> - struct dst_entry *dst = &rt->dst;
> -
> - memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
> + memset(&rt->init, 0, sizeof(rt->init));
> INIT_LIST_HEAD(&rt->rt6i_uncached);
> }
>