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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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;
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
- * - 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 <[email protected]>,
- * Matthew Hawkins <[email protected]>
- * - 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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, ð->h_dest, sizeof(*key_eth_addrs));
+ BUILD_BUG_ON(sizeof(*key_eth_addrs) != sizeof(eth->addrs));
+ memcpy(key_eth_addrs, ð->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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Gustavo A. R. Silva <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
As done for memcpy(), also update memmove() to use the same tightened
compile-time checks under CONFIG_FORTIFY_SOURCE.
Signed-off-by: Kees Cook <[email protected]>
---
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
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 <[email protected]>
---
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
Before changing anything about memcpy(), memmove(), and memset(), add
run-time tests to check basic behaviors for any regressions.
Signed-off-by: Kees Cook <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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)
+
+#include "test_fortify.h"
--
2.30.2
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 <[email protected]>
---
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
On Tue, Jul 27, 2021 at 02:18:58PM -0700, Nathan Chancellor wrote:
> 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 <[email protected]>
> > ---
> > 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 :)
/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
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<[email protected]>
Tested-by: Larry Finger <[email protected]>
Acked-by: Larry Finger <[email protected]>
Looks good.
Larry
On 7/27/21 1:58 PM, Kees Cook wrote:
> +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");
Has it been considered to implement this test using the Kunit framework?
Thanks,
Bart.
On 7/27/21 4:31 PM, Bart Van Assche wrote:
> On 7/27/21 1:58 PM, Kees Cook wrote:
>> +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");
>
> Has it been considered to implement this test using the Kunit framework?
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
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 <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> drivers/char/pcmcia/cm4000_cs.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
Acked-by: Greg Kroah-Hartman <[email protected]>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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
On Wed, Jul 28, 2021 at 01:14:33AM -0500, Gustavo A. R. Silva wrote:
>
>
> On 7/28/21 00:55, Greg Kroah-Hartman wrote:
> > 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 <[email protected]>
> >> ---
> >> 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 :)
>
> I think addrs should be second. In general, I think all newly added
> non-anonymous structures should be second.
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
On 7/28/21 00:55, Greg Kroah-Hartman wrote:
> 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 <[email protected]>
>> ---
>> 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 :)
I think addrs should be second. In general, I think all newly added
non-anonymous structures should be second.
Thanks
--
Gustavo
On 7/28/21 01:31, Gustavo A. R. Silva wrote:
>
>
> On 7/28/21 01:19, Greg Kroah-Hartman wrote:
>> On Wed, Jul 28, 2021 at 01:14:33AM -0500, Gustavo A. R. Silva wrote:
>>>
>>>
>>> On 7/28/21 00:55, Greg Kroah-Hartman wrote:
>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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 :)
>>>
>>> I think addrs should be second. In general, I think all newly added
>>> non-anonymous structures should be second.
>>
>> 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
Yep; I agree. That one looks just fine. :)
--
Gustavo
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 :)
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 <[email protected]>
> ---
> 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 <[email protected]>
On 28/07/2021 07.49, Greg Kroah-Hartman wrote:
> 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 <[email protected]>
>> ---
>> 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?
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
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
On Wed, Jul 28, 2021 at 11:42:15AM +0200, David Sterba wrote:
> 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 <[email protected]>
> > ---
> > 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);
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 <[email protected]>
What do you think?
-Kees
--
Kees Cook
On 7/28/21 2:01 PM, Kees Cook wrote:
> 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...)
How about using two memset() calls to clear h_dest[] and h_source[]
instead of modifying the uapi header?
Thanks,
Bart.
On Wed, Jul 28, 2021 at 02:56:31PM -0700, Kees Cook wrote:
> On Wed, Jul 28, 2021 at 11:42:15AM +0200, David Sterba wrote:
> > 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 <[email protected]>
> > > ---
> > > 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);
>
> Perhaps there should be another helper memset_starting()? That would
> make these cases a bit more self-documenting.
That would be better, yes.
> + memset_starting(item, 0, generation_v2);
memset_from?
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);
> }
>
On Wed, Jul 28, 2021 at 01:24:01PM +0200, Rasmus Villemoes wrote:
> On 28/07/2021 07.49, Greg Kroah-Hartman wrote:
> > 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 <[email protected]>
> >> ---
> >> 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?
>
> 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.
Ah, good call. I've adjusted this and added a comment.
Thanks!
-Kees
--
Kees Cook
On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote:
> 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 <[email protected]>
> > ---
> > 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)?
I went this direction because using two memset()s (or memcpy()s in other
patches) changes the machine code. It's not much of a change, but it
seems easier to justify "no binary changes" via the use of struct_group().
If splitting the memset() is preferred, I can totally do that instead.
:)
-Kees
--
Kees Cook
On 7/29/21 7:31 PM, Kees Cook wrote:
> On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote:
>> 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 <[email protected]>
>>> ---
>>> 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)?
>
> I went this direction because using two memset()s (or memcpy()s in other
> patches) changes the machine code. It's not much of a change, but it
> seems easier to justify "no binary changes" via the use of struct_group().
>
> If splitting the memset() is preferred, I can totally do that instead.
> :)
I don't have a strong opinion about this. Lars, do you want to comment
on this patch?
Thanks,
Bart.
On Thu, Jul 29, 2021 at 07:57:47PM -0700, Bart Van Assche wrote:
> On 7/29/21 7:31 PM, Kees Cook wrote:
> > On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote:
> >> 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 <[email protected]>
> >>> ---
> >>> 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)?
> >
> > I went this direction because using two memset()s (or memcpy()s in other
> > patches) changes the machine code. It's not much of a change, but it
> > seems easier to justify "no binary changes" via the use of struct_group().
> >
> > If splitting the memset() is preferred, I can totally do that instead.
> > :)
>
> I don't have a strong opinion about this. Lars, do you want to comment
> on this patch?
Fine either way. "no binary changes" sounds good ;-)
Thanks,
Lars
On Thu, Jul 29, 2021 at 7:31 PM Kees Cook <[email protected]> wrote:
>
> On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote:
> > 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 <[email protected]>
> > > ---
> > > 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)?
>
> I went this direction because using two memset()s (or memcpy()s in other
> patches) changes the machine code. It's not much of a change, but it
> seems easier to justify "no binary changes" via the use of struct_group().
>
> If splitting the memset() is preferred, I can totally do that instead.
> :)
I'm not sure that compilers can fold memsets of adjacent members. It
might not matter, but you could wrap these members in a _named_ struct
then simply use assignment for optimal codegen.
--
Thanks,
~Nick Desaulniers
On Thu, Jul 29, 2021 at 11:58:50AM -0700, Jakub Kicinski wrote:
> 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() ?
Oh, hah. Yes. I will adjust for v2.
--
Kees Cook
On Tue, Jul 27, 2021 at 01:58:30PM -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.
>
> 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.
Quick ping on this question: there is a mismatch between the common
helper and the other places that do this. Is there a bug here?
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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
On Tue, Jul 27, 2021 at 01:58:33PM -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 <[email protected]>
> ---
> 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?
Quick ping on this question.
The old code seems to be doing the wrong thing: it starts from the second
member and writes beyond int_info, clobbering qede_lock:
struct qede_dev {
...
struct qed_int_info int_info;
/* Smaller private variant of the RTNL lock */
struct mutex qede_lock;
...
struct qed_int_info {
struct msix_entry *msix;
u8 msix_cnt;
/* This should be updated by the protocol driver */
u8 used_cnt;
};
Should this also clear the "msix" member, or should this not write
beyond int_info? This patch does the latter.
-Kees
> ---
> 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
On Tue, Jul 31, 2021 at 07:07:00PM -0300, Kees Cook wrote:
> On Tue, Jul 27, 2021 at 01:58:33PM -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 <[email protected]>
> > ---
> > 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?
>
> Quick ping on this question.
>
> The old code seems to be doing the wrong thing: it starts from the second
> member and writes beyond int_info, clobbering qede_lock:
Thanks for highlighting the problem, but actually, the memset is redundant.
We will remove it so the change will not be needed.
>
> struct qede_dev {
> ...
> struct qed_int_info int_info;
>
> /* Smaller private variant of the RTNL lock */
> struct mutex qede_lock;
> ...
>
>
> struct qed_int_info {
> struct msix_entry *msix;
> u8 msix_cnt;
>
> /* This should be updated by the protocol driver */
> u8 used_cnt;
> };
>
> Should this also clear the "msix" member, or should this not write
> beyond int_info? This patch does the latter.
It should clear only the msix_cnt, no need to clear the entire
qed_int_info structure.
>
> -Kees
>
> > ---
> > 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);
We will replace the redundant memset with:
edev->int_info.msix_cnt = 0;
> > err3:
> > qede_napi_disable_remove(edev);
> > err2:
> > --
> > 2.30.2
> >
>
> --
> Kees Cook
On Mon, Aug 02, 2021 at 02:29:28PM +0000, Shai Malin wrote:
>
> On Tue, Jul 31, 2021 at 07:07:00PM -0300, Kees Cook wrote:
> > On Tue, Jul 27, 2021 at 01:58:33PM -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 <[email protected]>
> > > ---
> > > 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?
> >
> > Quick ping on this question.
> >
> > The old code seems to be doing the wrong thing: it starts from the second
> > member and writes beyond int_info, clobbering qede_lock:
>
> Thanks for highlighting the problem, but actually, the memset is redundant.
> We will remove it so the change will not be needed.
>
> >
> > struct qede_dev {
> > ...
> > struct qed_int_info int_info;
> >
> > /* Smaller private variant of the RTNL lock */
> > struct mutex qede_lock;
> > ...
> >
> >
> > struct qed_int_info {
> > struct msix_entry *msix;
> > u8 msix_cnt;
> >
> > /* This should be updated by the protocol driver */
> > u8 used_cnt;
> > };
> >
> > Should this also clear the "msix" member, or should this not write
> > beyond int_info? This patch does the latter.
>
> It should clear only the msix_cnt, no need to clear the entire
> qed_int_info structure.
Should used_cnt be cleared too? It is currently. Better yet, what patch
do you suggest I replace this proposed one with? :)
Thanks for looking at this!
-Kees
--
Kees Cook
On Mon, Aug 02, 2021 at 07:23:00PM +0300, Kees Cook wrote:
> On Mon, Aug 02, 2021 at 02:29:28PM +0000, Shai Malin wrote:
> > On Tue, Jul 31, 2021 at 07:07:00PM -0300, Kees Cook wrote:
> > > On Tue, Jul 27, 2021 at 01:58:33PM -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 <[email protected]>
> > > > ---
> > > > 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?
> > >
> > > Quick ping on this question.
> > >
> > > The old code seems to be doing the wrong thing: it starts from the second
> > > member and writes beyond int_info, clobbering qede_lock:
> >
> > Thanks for highlighting the problem, but actually, the memset is redundant.
> > We will remove it so the change will not be needed.
> >
> > >
> > > struct qede_dev {
> > > ...
> > > struct qed_int_info int_info;
> > >
> > > /* Smaller private variant of the RTNL lock */
> > > struct mutex qede_lock;
> > > ...
> > >
> > >
> > > struct qed_int_info {
> > > struct msix_entry *msix;
> > > u8 msix_cnt;
> > >
> > > /* This should be updated by the protocol driver */
> > > u8 used_cnt;
> > > };
> > >
> > > Should this also clear the "msix" member, or should this not write
> > > beyond int_info? This patch does the latter.
> >
> > It should clear only the msix_cnt, no need to clear the entire
> > qed_int_info structure.
>
> Should used_cnt be cleared too? It is currently. Better yet, what patch
> do you suggest I replace this proposed one with? :)
In qede_sync_free_irqs(), just after:
edev->int_info.used_cnt = 0;
Please add:
edev->int_info.msix_cnt = 0;
Thanks!
>
> Thanks for looking at this!
>
> -Kees
>
> --
> Kees Cook
On Sat, 2021-07-31 at 08:55 -0700, Kees Cook wrote:
> On Tue, Jul 27, 2021 at 01:58:30PM -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.
> >
> > 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.
>
> Quick ping on this question: there is a mismatch between the common
> helper and the other places that do this. Is there a bug here?
Yes.
The common helper should also clear ack_signal, but that was broken by
commit e3e1a0bcb3f1 ("mac80211: reduce IEEE80211_TX_MAX_RATES"), because
that commit changed the order of the fields and updated carl9170 and p54
properly but not the common helper...
It doesn't actually matter much because ack_signal is normally filled in
afterwards, and even if it isn't, it's just for statistics.
The correct thing to do here would be to
memset_after(&info->status, 0, rates);
johannes
On Sat, 2021-07-31 at 08:55 -0700, Kees Cook wrote:
>
> > @@ -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);
FWIW, I think we should also remove the BUILD_BUG_ON() now in all the
places - that was meant to give people a hint to update if some field
ordering etc. changed, but now that it's "after rates" this is no longer
necessary.
johannes
On Tue, 2021-07-27 at 13:58 -0700, Kees Cook wrote:
>
> +++ 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);
This file isn't really just lib80211, it's also used by everyone else
for 802.11, but I guess that's OK - after all, this doesn't really
result in any changes here.
> +++ 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);
However, how is it you don't need the same change in net/mac80211/wpa.c?
We have three similar instances:
/* AAD (extra authenticate-only data) / masked 802.11 header
* FC | A1 | A2 | A3 | SC | [A4] | [QC] */
put_unaligned_be16(len_a, &aad[0]);
put_unaligned(mask_fc, (__le16 *)&aad[2]);
memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);
and
memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);
and
memcpy(aad + 2, &hdr->addr1, 3 * ETH_ALEN);
so those should also be changed, it seems?
In which case I'd probably prefer to do this separately from the staging
drivers ...
johannes
On Fri, Aug 13, 2021 at 10:04:09AM +0200, Johannes Berg wrote:
> On Tue, 2021-07-27 at 13:58 -0700, Kees Cook wrote:
> >
> > +++ 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);
>
> This file isn't really just lib80211, it's also used by everyone else
> for 802.11, but I guess that's OK - after all, this doesn't really
> result in any changes here.
>
> > +++ 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);
>
>
> However, how is it you don't need the same change in net/mac80211/wpa.c?
>
> We have three similar instances:
>
> /* AAD (extra authenticate-only data) / masked 802.11 header
> * FC | A1 | A2 | A3 | SC | [A4] | [QC] */
> put_unaligned_be16(len_a, &aad[0]);
> put_unaligned(mask_fc, (__le16 *)&aad[2]);
> memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);
>
>
> and
>
> memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);
>
> and
>
> memcpy(aad + 2, &hdr->addr1, 3 * ETH_ALEN);
>
> so those should also be changed, it seems?
Ah! Yes, thanks for pointing this out. During earlier development I split
the "cross-field write" changes from the "cross-field read" changes, and
it looks like I missed moving lib80211_crypt_ccmp.c into that portion of
the series (which I haven't posted nor finished -- it's lower priority
than fixing the cross-field writes).
> In which case I'd probably prefer to do this separately from the staging
> drivers ...
Agreed. Sorry for the noise on that part. I will double-check the other
patches.
--
Kees Cook
On Fri, Aug 13, 2021 at 09:40:07AM +0200, Johannes Berg wrote:
> On Sat, 2021-07-31 at 08:55 -0700, Kees Cook wrote:
> > On Tue, Jul 27, 2021 at 01:58:30PM -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.
> > >
> > > 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.
> >
> > Quick ping on this question: there is a mismatch between the common
> > helper and the other places that do this. Is there a bug here?
>
> Yes.
>
> The common helper should also clear ack_signal, but that was broken by
> commit e3e1a0bcb3f1 ("mac80211: reduce IEEE80211_TX_MAX_RATES"), because
> that commit changed the order of the fields and updated carl9170 and p54
> properly but not the common helper...
It looks like p54 actually uses the rates, which is why it does this
manually. I can't see why carl9170 does this manually, though.
> It doesn't actually matter much because ack_signal is normally filled in
> afterwards, and even if it isn't, it's just for statistics.
>
> The correct thing to do here would be to
>
> memset_after(&info->status, 0, rates);
Sounds good; I will adjust these (and drop the BULID_BUG_ONs, as you
suggest in the next email).
Thanks!
-Kees
--
Kees Cook
On Fri, 2021-08-13 at 09:08 -0700, Kees Cook wrote:
> >
> > The common helper should also clear ack_signal, but that was broken by
> > commit e3e1a0bcb3f1 ("mac80211: reduce IEEE80211_TX_MAX_RATES"), because
> > that commit changed the order of the fields and updated carl9170 and p54
> > properly but not the common helper...
>
> It looks like p54 actually uses the rates, which is why it does this
> manually. I can't see why carl9170 does this manually, though.
mac80211 also uses the rates later again on status reporting, it just
expects the # of attempts to be filled etc. I haven't looked at
carl9170, but I would expect it to do something there and do it
correctly, even though old it's a well-written driver and uses mac80211
rate control, so this would need to be correct for decent performance.
But I guess it could be that the helper could be used because the rates
were already handed to the firmware, and the code was just copy/pasted
from p54 (the drivers were, IIRC, developed by the same folks)
johannes
On Fri, 2021-08-13 at 08:49 -0700, Kees Cook wrote:
>
> Ah! Yes, thanks for pointing this out. During earlier development I split
> the "cross-field write" changes from the "cross-field read" changes, and
> it looks like I missed moving lib80211_crypt_ccmp.c into that portion of
> the series (which I haven't posted nor finished -- it's lower priority
> than fixing the cross-field writes).
Oh, OK. I think all of this patch was cross-field read though.
Anyway, the patch itself is fine, just seems incomplete and somewhat
badly organised :)
johannes