[PATCH 03/64] rpmsg: glink: Replace strncpy() with strscpy_pad()
The use of strncpy() is considered deprecated for NUL-terminated
strings[1]. Replace strncpy() with strscpy_pad() (as it seems this case
expects the NUL padding to fill the allocation following the flexible
array). This additionally silences a warning seen when building under
-Warray-bounds:
./include/linux/fortify-string.h:38:30: warning: '__builtin_strncpy' offset 24
from the object at '__mptr' is out of the bounds of referenced subobject 'data'
with type 'u8[]' {aka 'unsigned char[]'} at offset 24 [-Warray-bounds]
38 | #define __underlying_strncpy __builtin_strncpy
| ^
./include/linux/fortify-string.h:50:9: note: in expansion of macro
'__underlying_strncpy'
50 | return __underlying_strncpy(p, q, size);
| ^~~~
drivers/rpmsg/qcom_glink_native.c: In function 'qcom_glink_work':
drivers/rpmsg/qcom_glink_native.c:36:5: note: subobject 'data' declared here
36 | u8 data[];
| ^~~~
[1]
https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
Signed-off-by: Kees Cook
---
drivers/rpmsg/qcom_glink_native.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rpmsg/qcom_glink_native.c
b/drivers/rpmsg/qcom_glink_native.c
index 05533c71b10e..c7b9de655080 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1440,7 +1440,7 @@ static int qcom_glink_rx_open(struct qcom_glink *glink,
unsigned int rcid,
}
rpdev->ept = &channel->ept;
- strncpy(rpdev->id.name, name, RPMSG_NAME_SIZE);
+ strscpy_pad(rpdev->id.name, name, RPMSG_NAME_SIZE);
rpdev->src = RPMSG_ADDR_ANY;
rpdev->dst = RPMSG_ADDR_ANY;
rpdev->ops = &glink_device_ops;
--
2.30.2
[PATCH 00/64] Introduce strict memcpy() bounds checking
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 m
[PATCH 01/64] media: omap3isp: Extract struct group for memcpy() region
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields. Wrap the target region
in a common named structure. This additionally fixes a theoretical
misalignment of the copy (since the size of "buf" changes between 64-bit
and 32-bit, but this is likely never built for 64-bit).
FWIW, I think this code is totally broken on 64-bit (which appears to
not be a "real" build configuration): it would either always fail (with
an uninitialized data->buf_size) or would cause corruption in userspace
due to the copy_to_user() in the call path against an uninitialized
data->buf value:
omap3isp_stat_request_statistics_time32(...)
struct omap3isp_stat_data data64;
...
omap3isp_stat_request_statistics(stat, &data64);
int omap3isp_stat_request_statistics(struct ispstat *stat,
struct omap3isp_stat_data *data)
...
buf = isp_stat_buf_get(stat, data);
static struct ispstat_buffer *isp_stat_buf_get(struct ispstat *stat,
struct omap3isp_stat_data *data)
...
if (buf->buf_size > data->buf_size) {
...
return ERR_PTR(-EINVAL);
}
...
rval = copy_to_user(data->buf,
buf->virt_addr,
buf->buf_size);
Regardless, additionally initialize data64 to be zero-filled to avoid
undefined behavior.
Fixes: 378e3f81cb56 ("media: omap3isp: support 64-bit version of
omap3isp_stat_data")
Signed-off-by: Kees Cook
---
drivers/media/platform/omap3isp/ispstat.c | 5 +--
include/uapi/linux/omap3isp.h | 44 +--
2 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/omap3isp/ispstat.c
b/drivers/media/platform/omap3isp/ispstat.c
index 5b9b57f4d9bf..ea8222fed38e 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -512,7 +512,7 @@ int omap3isp_stat_request_statistics(struct ispstat *stat,
int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
struct omap3isp_stat_data_time32 *data)
{
- struct omap3isp_stat_data data64;
+ struct omap3isp_stat_data data64 = { };
int ret;
ret = omap3isp_stat_request_statistics(stat, &data64);
@@ -521,7 +521,8 @@ int omap3isp_stat_request_statistics_time32(struct ispstat
*stat,
data->ts.tv_sec = data64.ts.tv_sec;
data->ts.tv_usec = data64.ts.tv_usec;
- memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
+ data->buf = (uintptr_t)data64.buf;
+ memcpy(&data->frame, &data64.buf, sizeof(data->frame));
return 0;
}
diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h
index 87b55755f4ff..0a16af91621f 100644
--- a/include/uapi/linux/omap3isp.h
+++ b/include/uapi/linux/omap3isp.h
@@ -159,13 +159,25 @@ struct omap3isp_h3a_aewb_config {
};
/**
- * struct omap3isp_stat_data - Statistic data sent to or received from user
- * @ts: Timestamp of returned framestats.
- * @buf: Pointer to pass to user.
+ * struct omap3isp_stat_frame - Statistic data without timestamp nor pointer.
+ * @buf_size: Size of buffer.
* @frame_number: Frame number of requested stats.
* @cur_frame: Current frame number being processed.
* @config_counter: Number of the configuration associated with the data.
*/
+struct omap3isp_stat_frame {
+ __u32 buf_size;
+ __u16 frame_number;
+ __u16 cur_frame;
+ __u16 config_counter;
+};
+
+/**
+ * struct omap3isp_stat_data - Statistic data sent to or received from user
+ * @ts: Timestamp of returned framestats.
+ * @buf: Pointer to pass to user.
+ * @frame: Statistic data for frame.
+ */
struct omap3isp_stat_data {
#ifdef __KERNEL__
struct {
@@ -176,10 +188,15 @@ struct omap3isp_stat_data {
struct timeval ts;
#endif
void __user *buf;
- __u32 buf_size;
- __u16 frame_number;
- __u16 cur_frame;
- __u16 config_counter;
+ union {
+ struct {
+ __u32 buf_size;
+ __u16 frame_number;
+ __u16 cur_frame;
+ __u16 config_counter;
+ };
+ struct omap3isp_stat_frame frame;
+ };
};
#ifdef __KERNEL__
@@ -189,10 +206,15 @@ struct omap3isp_stat_data_time32 {
__s32 tv_usec;
} ts;
__u32 buf;
- __u32 buf_size;
- __u16 frame_number;
- __u16 cur_frame;
- __u16 config_counter;
+ union {
+ struct {
+ __u32 buf_size;
+ __u16 frame_number;
+ __u16 cur_frame;
+
[PATCH 02/64] mac80211: Use flex-array for radiotap header bitmap
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
The it_present member of struct ieee80211_radiotap_header is treated as a
flexible array (multiple u32s can be conditionally present). In order for
memcpy() to reason (or really, not reason) about the size of operations
against this struct, use of bytes beyond it_present need to be treated
as part of the flexible array. Add a union/struct to contain the new
"bitmap" member, for use with trailing presence bitmaps and arguments.
Additionally improve readability in the iterator code which walks
through the bitmaps and arguments.
Signed-off-by: Kees Cook
---
include/net/ieee80211_radiotap.h | 24
net/mac80211/rx.c| 2 +-
net/wireless/radiotap.c | 5 ++---
3 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/include/net/ieee80211_radiotap.h b/include/net/ieee80211_radiotap.h
index c0854933e24f..101c1e961032 100644
--- a/include/net/ieee80211_radiotap.h
+++ b/include/net/ieee80211_radiotap.h
@@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {
*/
__le16 it_len;
- /**
-* @it_present: (first) present word
-*/
- __le32 it_present;
+ union {
+ /**
+* @it_present: (first) present word
+*/
+ __le32 it_present;
+
+ struct {
+ /* The compiler makes it difficult to overlap
+* a flex-array with an existing singleton,
+* so we're forced to add an empty named
+* variable here.
+*/
+ struct { } __unused;
+
+ /**
+* @bitmap: all presence bitmaps
+*/
+ __le32 bitmap[];
+ };
+ };
} __packed;
/* version is always 0 */
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 2563473b5cf1..0d959a98e908 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -328,7 +328,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local
*local,
rthdr = skb_push(skb, rtap_len);
memset(rthdr, 0, rtap_len - rtap.len - rtap.pad);
- it_present = &rthdr->it_present;
+ it_present = rthdr->bitmap;
/* radiotap header, set always present flags */
rthdr->it_len = cpu_to_le16(rtap_len);
diff --git a/net/wireless/radiotap.c b/net/wireless/radiotap.c
index 36f1b59a78bf..9f4f1a772964 100644
--- a/net/wireless/radiotap.c
+++ b/net/wireless/radiotap.c
@@ -115,10 +115,9 @@ int ieee80211_radiotap_iterator_init(
iterator->_max_length = get_unaligned_le16(&radiotap_header->it_len);
iterator->_arg_index = 0;
iterator->_bitmap_shifter =
get_unaligned_le32(&radiotap_header->it_present);
- iterator->_arg = (uint8_t *)radiotap_header + sizeof(*radiotap_header);
+ iterator->_arg = (uint8_t *)&radiotap_header->bitmap[1];
iterator->_reset_on_ext = 0;
- iterator->_next_bitmap = &radiotap_header->it_present;
- iterator->_next_bitmap++;
+ iterator->_next_bitmap = &radiotap_header->bitmap[1];
iterator->_vns = vns;
iterator->current_namespace = &radiotap_ns;
iterator->is_radiotap_ns = 1;
--
2.30.2
[PATCH 04/64] stddef: Introduce struct_group() helper macro
Kernel code has a regular need to describe groups of members within a
structure usually when they need to be copied or initialized separately
from the rest of the surrounding structure. The generally accepted design
pattern in C is to use a named sub-struct:
struct foo {
int one;
struct {
int two;
int three;
} thing;
int four;
};
This would allow for traditional references and sizing:
memcpy(&dst.thing, &src.thing, sizeof(dst.thing));
However, doing this would mean that referencing struct members enclosed
by such named structs would always require including the sub-struct name
in identifiers:
do_something(dst.thing.three);
This has tended to be quite inflexible, especially when such groupings
need to be added to established code which causes huge naming churn.
Three workarounds exist in the kernel for this problem, and each have
other negative properties.
To avoid the naming churn, there is a design pattern of adding macro
aliases for the named struct:
#define f_three thing.three
This ends up polluting the global namespace, and makes it difficult to
search for identifiers.
Another common work-around in kernel code avoids the pollution by avoiding
the named struct entirely, instead identifying the group's boundaries using
either a pair of empty anonymous structs of a pair of zero-element arrays:
struct foo {
int one;
struct { } start;
int two;
int three;
struct { } finish;
int four;
};
struct foo {
int one;
int start[0];
int two;
int three;
int finish[0];
int four;
};
This allows code to avoid needing to use a sub-struct name for member
references within the surrounding structure, but loses the benefits of
being able to actually use such a struct, making it rather fragile. Using
these requires open-coded calculation of sizes and offsets. The efforts
made to avoid common mistakes include lots of comments, or adding various
BUILD_BUG_ON()s. Such code is left with no way for the compiler to reason
about the boundaries (e.g. the "start" object looks like it's 0 bytes
in length and is not structurally associated with "finish"), making bounds
checking depend on open-coded calculations:
if (length > offsetof(struct foo, finish) -
offsetof(struct foo, start))
return -EINVAL;
memcpy(&dst.start, &src.start, length);
However, the vast majority of places in the kernel that operate on
groups of members do so without any identification of the grouping,
relying either on comments or implicit knowledge of the struct contents,
which is even harder for the compiler to reason about, and results in
even more fragile manual sizing, usually depending on member locations
outside of the region (e.g. to copy "two" and "three", use the start of
"four" to find the size):
BUILD_BUG_ON((offsetof(struct foo, four) <
offsetof(struct foo, two)) ||
(offsetof(struct foo, four) <
offsetof(struct foo, three));
if (length > offsetof(struct foo, four) -
offsetof(struct foo, two))
return -EINVAL;
memcpy(&dst.two, &src.two, length);
And both of the prior two idioms additionally appear to write beyond the
end of the referenced struct member, forcing the compiler to ignore any
attempt to perform bounds checking.
In order to have a regular programmatic way to describe a struct
region that can be used for references and sizing, can be examined for
bounds checking, avoids forcing the use of intermediate identifiers,
and avoids polluting the global namespace, introduce the struct_group()
macro. This macro wraps the member declarations to create an anonymous
union of an anonymous struct (no intermediate name) and a named struct
(for references and sizing):
struct foo {
int one;
struct_group(thing,
int two,
int three,
);
int four;
};
if (length > sizeof(src.thing))
return -EINVAL;
memcpy(&dst.thing, &src.thing, length);
do_something(dst.three);
There are some rare cases where the resulting struct_group() needs
attributes added, so struct_group_attr() is also introduced to allow
for specifying struct attributes (e.g. __align(x) or __packed).
Co-developed-by: Keith Packard
Signed-off-by: Keith Packard
Signed-off-by: Kees Cook
---
include/linux/stddef.h | 34 ++
1 file changed, 34 i
[PATCH 10/64] lib80211: Use struct_group() for memcpy() region
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
---
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
[PATCH 15/64] ipw2x00: Use struct_group() for memcpy() region
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field array bounds checking for memcpy(), memmove(), and memset(),
avoid intentionally writing across neighboring fields.
Use struct_group() in struct libipw_qos_information_element around
members qui, qui_type, qui_subtype, version, and ac_info, so they can be
referenced together. This will allow memcpy() and sizeof() to more easily
reason about sizes, improve readability, and avoid future warnings about
writing beyond the end of qui.
"pahole" shows no size nor member offset changes to struct
libipw_qos_information_element.
Additionally corrects the size in libipw_read_qos_param_element() as
it was testing the wrong structure size (it should have been struct
libipw_qos_information_element, not struct libipw_qos_parameter_info).
Signed-off-by: Kees Cook
---
drivers/net/wireless/intel/ipw2x00/libipw.h| 12 +++-
drivers/net/wireless/intel/ipw2x00/libipw_rx.c | 8
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/intel/ipw2x00/libipw.h
b/drivers/net/wireless/intel/ipw2x00/libipw.h
index 7964ef7d15f0..4006a0db2eea 100644
--- a/drivers/net/wireless/intel/ipw2x00/libipw.h
+++ b/drivers/net/wireless/intel/ipw2x00/libipw.h
@@ -537,11 +537,13 @@ struct libipw_txb {
struct libipw_qos_information_element {
u8 elementID;
u8 length;
- u8 qui[QOS_OUI_LEN];
- u8 qui_type;
- u8 qui_subtype;
- u8 version;
- u8 ac_info;
+ struct_group(data,
+ u8 qui[QOS_OUI_LEN];
+ u8 qui_type;
+ u8 qui_subtype;
+ u8 version;
+ u8 ac_info;
+ );
} __packed;
struct libipw_qos_ac_parameter {
diff --git a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
index 5a2a723e480b..75cc3cab4992 100644
--- a/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
+++ b/drivers/net/wireless/intel/ipw2x00/libipw_rx.c
@@ -948,13 +948,13 @@ static int libipw_read_qos_param_element(struct
libipw_qos_parameter_info
*info_element)
{
int ret = 0;
- u16 size = sizeof(struct libipw_qos_parameter_info) - 2;
+ u16 size = sizeof(element_param->info_element.data);
if ((info_element == NULL) || (element_param == NULL))
return -1;
if (info_element->id == QOS_ELEMENT_ID && info_element->len == size) {
- memcpy(element_param->info_element.qui, info_element->data,
+ memcpy(&element_param->info_element.data, info_element->data,
info_element->len);
element_param->info_element.elementID = info_element->id;
element_param->info_element.length = info_element->len;
@@ -975,7 +975,7 @@ static int libipw_read_qos_info_element(struct
*info_element)
{
int ret = 0;
- u16 size = sizeof(struct libipw_qos_information_element) - 2;
+ u16 size = sizeof(element_info->data);
if (element_info == NULL)
return -1;
@@ -983,7 +983,7 @@ static int libipw_read_qos_info_element(struct
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
[PATCH 14/64] libertas_tf: Use struct_group() for memcpy() region
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
---
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
[PATCH 12/64] mwl8k: Use struct_group() for memcpy() region
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct mwl8k_cmd_set_key around members
key_material, tkip_tx_mic_key, and tkip_rx_mic_key so they can be
referenced together. This will allow memcpy() and sizeof() to more easily
reason about sizes, improve readability, and avoid future warnings about
writing beyond the end of key_material.
"pahole" shows no size nor member offset changes to struct
mwl8k_cmd_set_key. "objdump -d" shows no object code changes.
Signed-off-by: Kees Cook
---
drivers/net/wireless/marvell/mwl8k.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwl8k.c
b/drivers/net/wireless/marvell/mwl8k.c
index 3bf6571f4149..a29277d5f9da 100644
--- a/drivers/net/wireless/marvell/mwl8k.c
+++ b/drivers/net/wireless/marvell/mwl8k.c
@@ -4225,9 +4225,11 @@ struct mwl8k_cmd_set_key {
__le32 key_info;
__le32 key_id;
__le16 key_len;
- __u8 key_material[MAX_ENCR_KEY_LENGTH];
- __u8 tkip_tx_mic_key[MIC_KEY_LENGTH];
- __u8 tkip_rx_mic_key[MIC_KEY_LENGTH];
+ struct {
+ __u8 key_material[MAX_ENCR_KEY_LENGTH];
+ __u8 tkip_tx_mic_key[MIC_KEY_LENGTH];
+ __u8 tkip_rx_mic_key[MIC_KEY_LENGTH];
+ } tkip;
__le16 tkip_rsc_low;
__le32 tkip_rsc_high;
__le16 tkip_tsc_low;
@@ -4375,7 +4377,7 @@ static int mwl8k_cmd_encryption_set_key(struct
ieee80211_hw *hw,
goto done;
}
- memcpy(cmd->key_material, key->key, keymlen);
+ memcpy(&cmd->tkip, key->key, keymlen);
cmd->action = cpu_to_le32(action);
rc = mwl8k_post_pervif_cmd(hw, vif, &cmd->header);
--
2.30.2
[PATCH 11/64] net/mlx5e: Avoid field-overflowing memcpy()
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
---
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_mttinline_mtts[0];
+ struct mlx5_mttinline_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
[PATCH 08/64] staging: rtl8192u: Use struct_group() for memcpy() region
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
---
.../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(
s
[PATCH 31/64] fortify: Explicitly disable Clang support
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 --- 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
[PATCH 29/64] lib/string: Move helper functions out of string.c
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
---
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
#include
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
#include
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
+ * This file should be used only for "library" routines that may have
+ * alternative implementations on specific architectures (generally
+ * found in ), 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
- * - 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 ,
- *Matthew Hawkins
- * - Kissed strtok() goodbye
+ * Other helper functions should live in string_helpers.c.
*/
+#define __NO_FORTIFY
#include
#include
#include
@@ -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_str
[PATCH 18/64] cxgb3: Use struct_group() for memcpy() region
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
---
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
[PATCH 30/64] fortify: Move remaining fortify helpers into fortify-string.h
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
---
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
#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
[PATCH 27/64] HID: cp2112: Use struct_group() for memcpy() region
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct cp2112_string_report around members report,
length, type, and string, so they can be referenced together. This will
allow memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
report.
"pahole" shows no size nor member offset changes to struct
cp2112_string_report. "objdump -d" shows no meaningful object
code changes (i.e. only source line number induced differences.)
Signed-off-by: Kees Cook
---
drivers/hid/hid-cp2112.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 477baa30889c..e6ee453c7cfc 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -129,10 +129,12 @@ struct cp2112_xfer_status_report {
struct cp2112_string_report {
u8 dummy; /* force .string to be aligned */
- u8 report; /* CP2112_*_STRING */
- u8 length; /* length in bytes of everyting after .report */
- u8 type;/* USB_DT_STRING */
- wchar_t string[30]; /* UTF16_LITTLE_ENDIAN string */
+ struct_group_attr(contents, __packed,
+ u8 report; /* CP2112_*_STRING */
+ u8 length; /* length in bytes of everyting after
.report */
+ u8 type;/* USB_DT_STRING */
+ wchar_t string[30]; /* UTF16_LITTLE_ENDIAN string */
+ );
} __packed;
/* Number of times to request transfer status before giving up waiting for a
@@ -986,8 +988,8 @@ static ssize_t pstr_show(struct device *kdev,
u8 length;
int ret;
- ret = cp2112_hid_get(hdev, attr->report, &report.report,
-sizeof(report) - 1, HID_FEATURE_REPORT);
+ ret = cp2112_hid_get(hdev, attr->report, (u8 *)&report.contents,
+sizeof(report.contents), HID_FEATURE_REPORT);
if (ret < 3) {
hid_err(hdev, "error reading %s string: %d\n", kattr->attr.name,
ret);
--
2.30.2
[PATCH 28/64] compiler_types.h: Remove __compiletime_object_size()
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
Signed-off-by: Kees Cook
---
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
[PATCH 06/64] bnxt_en: Use struct_group_attr() for memcpy() region
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
---
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
[PATCH 09/64] staging: rtl8723bs: Avoid field-overflowing memcpy()
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memcpy(), memmove(), and memset(), avoid intentionally writing across neighboring fields. Adjust memcpy() destination to be the named structure itself, rather than the first member, allowing memcpy() to correctly reason about the size. "objdump -d" shows no object code changes. Signed-off-by: Kees Cook --- drivers/staging/rtl8723bs/core/rtw_mlme.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c index a0c19253095d..fbd6e3d16323 100644 --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c @@ -2390,7 +2390,7 @@ unsigned int rtw_restructure_ht_ie(struct adapter *padapter, u8 *in_ie, u8 *out_ } /* fill default supported_mcs_set */ - memcpy(ht_capie.mcs.rx_mask, pmlmeext->default_supported_mcs_set, 16); + memcpy(&ht_capie.mcs, pmlmeext->default_supported_mcs_set, 16); /* update default supported_mcs_set */ rtw_hal_get_hwreg(padapter, HW_VAR_RF_TYPE, (u8 *)(&rf_type)); -- 2.30.2
[PATCH 32/64] fortify: Add compile-time FORTIFY_SOURCE tests
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 --- 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 > $@ || 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 ..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 ..b26e1f1bc217 --- /dev/null +++ b/lib/test_fortify/read_overflow-memchr_inv.c @
[PATCH 07/64] staging: rtl8192e: Use struct_group() for memcpy() region
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
---
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
[PATCH 22/64] bnx2x: Use struct_group() for memcpy() region
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
---
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
[PATCH 47/64] btrfs: Use memset_after() to clear end of struct
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
---
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
[PATCH 05/64] skbuff: Switch structure bounds to struct_group()
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Replace the existing empty member position markers "headers_start" and
"headers_end" with a struct_group(). This will allow memcpy() and sizeof()
to more easily reason about sizes, and improve readability.
"pahole" shows no size nor member offset changes to struct sk_buff.
"objdump -d" shows no no meaningful object code changes (i.e. only source
line number induced differences and optimizations.)
Signed-off-by: Kees Cook
---
drivers/net/wireguard/queueing.h | 4 +---
include/linux/skbuff.h | 9 -
net/core/skbuff.c| 14 +-
3 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h
index 4ef2944a68bc..52da5e963003 100644
--- a/drivers/net/wireguard/queueing.h
+++ b/drivers/net/wireguard/queueing.h
@@ -79,9 +79,7 @@ static inline void wg_reset_packet(struct sk_buff *skb, bool
encapsulating)
u8 sw_hash = skb->sw_hash;
u32 hash = skb->hash;
skb_scrub_packet(skb, true);
- memset(&skb->headers_start, 0,
- offsetof(struct sk_buff, headers_end) -
- offsetof(struct sk_buff, headers_start));
+ memset(&skb->headers, 0, sizeof(skb->headers));
if (encapsulating) {
skb->l4_hash = l4_hash;
skb->sw_hash = sw_hash;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f19190820e63..b4032e9b130e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -800,11 +800,10 @@ struct sk_buff {
__u8active_extensions;
#endif
- /* fields enclosed in headers_start/headers_end are copied
+ /* Fields enclosed in headers group are copied
* using a single memcpy() in __copy_skb_header()
*/
- /* private: */
- __u32 headers_start[0];
+ struct_group(headers,
/* public: */
/* if you move pkt_type around you also must adapt those constants */
@@ -920,8 +919,8 @@ struct sk_buff {
u64 kcov_handle;
#endif
- /* private: */
- __u32 headers_end[0];
+ ); /* end headers group */
+
/* public: */
/* These elements must be at the end, see alloc_skb() for details. */
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index fc7942c0dddc..5f29c65507e0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -987,12 +987,10 @@ void napi_consume_skb(struct sk_buff *skb, int budget)
}
EXPORT_SYMBOL(napi_consume_skb);
-/* Make sure a field is enclosed inside headers_start/headers_end section */
+/* Make sure a field is contained by headers group */
#define CHECK_SKB_FIELD(field) \
- BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
-offsetof(struct sk_buff, headers_start)); \
- BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
-offsetof(struct sk_buff, headers_end));\
+ BUILD_BUG_ON(offsetof(struct sk_buff, field) != \
+offsetof(struct sk_buff, headers.field)); \
static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
{
@@ -1004,14 +1002,12 @@ static void __copy_skb_header(struct sk_buff *new,
const struct sk_buff *old)
__skb_ext_copy(new, old);
__nf_copy(new, old, false);
- /* Note : this field could be in headers_start/headers_end section
+ /* Note : this field could be in the headers group.
* It is not yet because we do not want to have a 16 bit hole
*/
new->queue_mapping = old->queue_mapping;
- memcpy(&new->headers_start, &old->headers_start,
- offsetof(struct sk_buff, headers_end) -
- offsetof(struct sk_buff, headers_start));
+ memcpy(&new->headers, &old->headers, sizeof(new->headers));
CHECK_SKB_FIELD(protocol);
CHECK_SKB_FIELD(csum);
CHECK_SKB_FIELD(hash);
--
2.30.2
[PATCH 13/64] libertas: Use struct_group() for memcpy() region
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
---
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
[PATCH 26/64] net/mlx5e: Use struct_group() for memcpy() region
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
---
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
[PATCH 64/64] fortify: Add run-time WARN for cross-field memcpy()
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
---
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
[PATCH 25/64] drm/mga/mga_ioc32: Use struct_group() for memcpy() region
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct drm32_mga_init around members chipset, sgram,
maccess, fb_cpp, front_offset, front_pitch, back_offset, back_pitch,
depth_cpp, depth_offset, depth_pitch, texture_offset, and texture_size,
so they can be referenced together. This will allow memcpy() and sizeof()
to more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of chipset.
"pahole" shows no size nor member offset changes to struct drm32_mga_init.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Note that since this includes a UAPI header, struct_group() has been
explicitly redefined local to the header.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/mga/mga_ioc32.c | 30 ++
include/uapi/drm/mga_drm.h | 37 -
2 files changed, 44 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/mga/mga_ioc32.c b/drivers/gpu/drm/mga/mga_ioc32.c
index 4fd4de16cd32..fbd0329dbd4f 100644
--- a/drivers/gpu/drm/mga/mga_ioc32.c
+++ b/drivers/gpu/drm/mga/mga_ioc32.c
@@ -38,16 +38,21 @@
typedef struct drm32_mga_init {
int func;
u32 sarea_priv_offset;
- int chipset;
- int sgram;
- unsigned int maccess;
- unsigned int fb_cpp;
- unsigned int front_offset, front_pitch;
- unsigned int back_offset, back_pitch;
- unsigned int depth_cpp;
- unsigned int depth_offset, depth_pitch;
- unsigned int texture_offset[MGA_NR_TEX_HEAPS];
- unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ struct_group(always32bit,
+ int chipset;
+ int sgram;
+ unsigned int maccess;
+ unsigned int fb_cpp;
+ unsigned int front_offset;
+ unsigned int front_pitch;
+ unsigned int back_offset;
+ unsigned int back_pitch;
+ unsigned int depth_cpp;
+ unsigned int depth_offset;
+ unsigned int depth_pitch;
+ unsigned int texture_offset[MGA_NR_TEX_HEAPS];
+ unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ );
u32 fb_offset;
u32 mmio_offset;
u32 status_offset;
@@ -67,9 +72,8 @@ static int compat_mga_init(struct file *file, unsigned int
cmd,
init.func = init32.func;
init.sarea_priv_offset = init32.sarea_priv_offset;
- memcpy(&init.chipset, &init32.chipset,
- offsetof(drm_mga_init_t, fb_offset) -
- offsetof(drm_mga_init_t, chipset));
+ memcpy(&init.always32bit, &init32.always32bit,
+ sizeof(init32.always32bit));
init.fb_offset = init32.fb_offset;
init.mmio_offset = init32.mmio_offset;
init.status_offset = init32.status_offset;
diff --git a/include/uapi/drm/mga_drm.h b/include/uapi/drm/mga_drm.h
index 8c4337548ab5..61612e5ecab2 100644
--- a/include/uapi/drm/mga_drm.h
+++ b/include/uapi/drm/mga_drm.h
@@ -265,6 +265,16 @@ typedef struct _drm_mga_sarea {
#define DRM_IOCTL_MGA_WAIT_FENCEDRM_IOWR(DRM_COMMAND_BASE +
DRM_MGA_WAIT_FENCE, __u32)
#define DRM_IOCTL_MGA_DMA_BOOTSTRAP DRM_IOWR(DRM_COMMAND_BASE +
DRM_MGA_DMA_BOOTSTRAP, drm_mga_dma_bootstrap_t)
+#define __struct_group(name, fields) \
+ union { \
+ struct { \
+ fields \
+ }; \
+ struct { \
+ fields \
+ } name; \
+ }
+
typedef struct _drm_mga_warp_index {
int installed;
unsigned long phys_addr;
@@ -279,20 +289,25 @@ typedef struct drm_mga_init {
unsigned long sarea_priv_offset;
- int chipset;
- int sgram;
+ __struct_group(always32bit,
+ int chipset;
+ int sgram;
- unsigned int maccess;
+ unsigned int maccess;
- unsigned int fb_cpp;
- unsigned int front_offset, front_pitch;
- unsigned int back_offset, back_pitch;
+ unsigned int fb_cpp;
+ unsigned int front_offset;
+ unsigned int front_pitch;
+ unsigned int back_offset;
+ unsigned int back_pitch;
- unsigned int depth_cpp;
- unsigned int depth_offset, depth_pitch;
+ unsigned int depth_cpp;
+ unsigned int depth_offset;
+ unsigned int depth_pitch;
- unsigned int texture_offset[MGA_NR_TEX_HEAPS];
- unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ unsigned int texture_offset[MGA_NR_TEX_HEAPS];
+ unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ );
unsigned long fb_offset;
unsigned long mmio
[PATCH 40/64] net: 802: Use memset_after() to clear struct fields
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 --- 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..00fb4b16 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
[PATCH 19/64] ip: Use struct_group() for memcpy() regions
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
---
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 {
__u8ttl;
__u8protocol;
__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 {
__u8nexthdr;
__u8hop_limit;
- struct in6_addrsaddr;
- struct in6_addrdaddr;
+ union {
+ struct {
+ struct in6_addrsaddr;
+ struct in6_addrdaddr;
+ } addrs;
+ struct {
+ struct in6_addrsaddr;
+ struct in6_addrdaddr;
+ };
+ };
};
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,
[PATCH 17/64] iommu/amd: Use struct_group() for memcpy() region
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct ivhd_entry around members ext and hidh, so
they can be referenced together. This will allow memcpy() and sizeof()
to more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of ext.
"pahole" shows no size nor member offset changes to struct ivhd_entry.
"objdump -d" shows no object code changes.
Signed-off-by: Kees Cook
---
drivers/iommu/amd/init.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 46280e6e1535..2df84737417b 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -121,8 +121,10 @@ struct ivhd_entry {
u8 type;
u16 devid;
u8 flags;
- u32 ext;
- u32 hidh;
+ struct_group(ext_hid,
+ u32 ext;
+ u32 hidh;
+ );
u64 cid;
u8 uidf;
u8 uidl;
@@ -1378,7 +1380,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
}
- memcpy(hid, (u8 *)(&e->ext), ACPIHID_HID_LEN - 1);
+ BUILD_BUG_ON(sizeof(e->ext_hid) != ACPIHID_HID_LEN - 1);
+ memcpy(hid, &e->ext_hid, ACPIHID_HID_LEN - 1);
hid[ACPIHID_HID_LEN - 1] = '\0';
if (!(*hid)) {
--
2.30.2
[PATCH 16/64] thermal: intel: int340x_thermal: Use struct_group() for memcpy() region
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
---
.../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
[PATCH 55/64] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr
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
---
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
[PATCH 42/64] net: qede: Use memset_after() for counters
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 --- 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
[PATCH 41/64] net: dccp: Use memset_after() for TP zeroing
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
---
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
[PATCH 44/64] iw_cxgb4: Use memset_after() for cpl_t5_pass_accept_rpl
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct. Additionally, since everything
appears to perform a roundup (including allocation), just change the
size of the struct itself and add a build-time check to validate the
expected size.
Signed-off-by: Kees Cook
---
drivers/infiniband/hw/cxgb4/cm.c| 5 +++--
drivers/net/ethernet/chelsio/cxgb4/t4_msg.h | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index 291471d12197..7129ae025b2d 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -2471,7 +2471,8 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff
*skb,
skb_get(skb);
rpl = cplhdr(skb);
if (!is_t4(adapter_type)) {
- skb_trim(skb, roundup(sizeof(*rpl5), 16));
+ BUILD_BUG_ON(sizeof(*rpl5) != roundup(sizeof(*rpl5), 16));
+ skb_trim(skb, sizeof(*rpl5));
rpl5 = (void *)rpl;
INIT_TP_WR(rpl5, ep->hwtid);
} else {
@@ -2487,7 +2488,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff
*skb,
opt2 |= CONG_CNTRL_V(CONG_ALG_TAHOE);
opt2 |= T5_ISS_F;
rpl5 = (void *)rpl;
- memset(&rpl5->iss, 0, roundup(sizeof(*rpl5)-sizeof(*rpl), 16));
+ memset_after(rpl5, 0, opt0);
if (peer2peer)
isn += 4;
rpl5->iss = cpu_to_be32(isn);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
index fed5f93bf620..26433a62d7f0 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h
@@ -497,7 +497,7 @@ struct cpl_t5_pass_accept_rpl {
__be32 opt2;
__be64 opt0;
__be32 iss;
- __be32 rsvd;
+ __be32 rsvd[3];
};
struct cpl_act_open_req {
--
2.30.2
[PATCH 38/64] xfrm: Use memset_after() to clear padding
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
---
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
[PATCH 39/64] mac80211: Use memset_after() to clear tx status
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 --- 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
[PATCH 43/64] ath11k: Use memset_after() for clearing queue descriptors
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 --- 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
[PATCH 63/64] iwlwifi: dbg_ini: Split memcpy() to avoid multi-field write
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
---
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_MAGIC0x0a4c5749
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
[PATCH 54/64] ipv6: Use struct_group() to zero rt6_info
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
---
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_entrydst;
- 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_headrt6i_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_headrt6i_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
[PATCH 53/64] HID: roccat: Use struct_group() to zero kone_mouse_event
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
---
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
[PATCH 61/64] Makefile: Enable -Warray-bounds
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 Signed-off-by: Gustavo A. R. Silva Signed-off-by: Kees Cook --- 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
[PATCH 56/64] ethtool: stats: Use struct_group() to clear all stats at once
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
---
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_statsphy_stats;
- struct ethtool_eth_mac_statsmac_stats;
- struct ethtool_eth_ctrl_stats ctrl_stats;
- struct ethtool_rmon_stats rmon_stats;
+ struct_group(stats,
+ struct ethtool_eth_phy_statsphy_stats;
+ struct ethtool_eth_mac_statsmac_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
[PATCH 50/64] KVM: x86: Use struct_group() to zero decode cache
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
---
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
[PATCH 37/64] string.h: Introduce memset_after() for wiping trailing members/padding
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
---
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
[PATCH 57/64] netfilter: conntrack: Use struct_group() to zero struct nf_conn
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Replace the empty __nfct_init_offset member with a struct_group() to
mark the region of struct nf_conn that should be initialized to zero,
allowing memset() to correctly reason about the size of the write.
Signed-off-by: Kees Cook
---
include/net/netfilter/nf_conntrack.h | 20 ++--
net/netfilter/nf_conntrack_core.c| 4 +---
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack.h
b/include/net/netfilter/nf_conntrack.h
index cc663c68ddc4..51004fce2937 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -101,24 +101,24 @@ struct nf_conn {
struct hlist_node nat_bysource;
#endif
/* all members below initialized via memset */
- struct { } __nfct_init_offset;
-
- /* If we were expected by an expectation, this will be it */
- struct nf_conn *master;
+ struct_group(init,
+ /* If we were expected by an expectation, this will be it */
+ struct nf_conn *master;
#if defined(CONFIG_NF_CONNTRACK_MARK)
- u_int32_t mark;
+ u_int32_t mark;
#endif
#ifdef CONFIG_NF_CONNTRACK_SECMARK
- u_int32_t secmark;
+ u_int32_t secmark;
#endif
- /* Extensions */
- struct nf_ct_ext *ext;
+ /* Extensions */
+ struct nf_ct_ext *ext;
- /* Storage reserved for other modules, must be the last member */
- union nf_conntrack_proto proto;
+ /* Storage reserved for other modules, must be the last member
*/
+ union nf_conntrack_proto proto;
+ );
};
static inline struct nf_conn *
diff --git a/net/netfilter/nf_conntrack_core.c
b/net/netfilter/nf_conntrack_core.c
index 5c03e5106751..b05eb64ec788 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1527,9 +1527,7 @@ __nf_conntrack_alloc(struct net *net,
ct->status = 0;
ct->timeout = 0;
write_pnet(&ct->ct_net, net);
- memset(&ct->__nfct_init_offset, 0,
- offsetof(struct nf_conn, proto) -
- offsetof(struct nf_conn, __nfct_init_offset));
+ memset(&ct->init, 0, sizeof(ct->init));
nf_ct_zone_add(ct, zone);
--
2.30.2
[PATCH 21/64] cxgb4: Use struct_group() for memcpy() region
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
---
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 1
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
[PATCH 60/64] fortify: Work around Clang inlining bugs
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
---
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_IN
[PATCH 36/64] scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memset(), avoid intentionally writing across neighboring fields. Instead of writing beyond the end of evt_struct->iu.srp.cmd, target the upper union (evt_struct->iu.srp) instead, as that's what is being wiped. Signed-off-by: Kees Cook --- drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c index e6a3eaaa57d9..7e8beb42d2d3 100644 --- a/drivers/scsi/ibmvscsi/ibmvscsi.c +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, return SCSI_MLQUEUE_HOST_BUSY; /* Set up the actual SRP IU */ + memset(&evt_struct->iu.srp, 0x00, SRP_MAX_IU_LEN); srp_cmd = &evt_struct->iu.srp.cmd; - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); srp_cmd->opcode = SRP_CMD; memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); int_to_scsilun(lun, &srp_cmd->lun); -- 2.30.2
[PATCH 62/64] netlink: Avoid false-positive memcpy() warning
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
---
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 */
+ __u8contents[];
};
/* 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
[PATCH 23/64] drm/amd/pm: Use struct_group() for memcpy() region
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
PPTable_t
so the grouped members can be referenced together. This will allow
memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
the first member.
"pahole" shows no size nor member offset changes to any structs.
"objdump -d" shows no object code changes.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/amd/include/atomfirmware.h | 9 -
.../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h| 3 ++-
drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++-
.../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++-
drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c| 6 +++---
drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 12
drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 +++---
7 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h
b/drivers/gpu/drm/amd/include/atomfirmware.h
index 3811e58dd857..694dee9d2691 100644
--- a/drivers/gpu/drm/amd/include/atomfirmware.h
+++ b/drivers/gpu/drm/amd/include/atomfirmware.h
@@ -2081,6 +2081,7 @@ struct atom_smc_dpm_info_v4_5
{
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
// I2C Control
struct smudpm_i2c_controller_config_v2 I2cControllers[8];
@@ -2159,7 +2160,7 @@ struct atom_smc_dpm_info_v4_5
uint32_t MvddRatio; // This is used for MVDD Vid workaround. It has 16
fractional bits (Q16.16)
uint32_t BoardReserved[9];
-
+ );
};
struct atom_smc_dpm_info_v4_6
@@ -2168,6 +2169,7 @@ struct atom_smc_dpm_info_v4_6
// section: board parameters
uint32_t i2c_padding[3]; // old i2c control are moved to new area
+ struct_group(dpm_info,
uint16_t maxvoltagestepgfx; // in mv(q2) max voltage step that smu will
request. multiple steps are taken if voltage change exceeds this value.
uint16_t maxvoltagestepsoc; // in mv(q2) max voltage step that smu will
request. multiple steps are taken if voltage change exceeds this value.
@@ -2246,12 +2248,14 @@ struct atom_smc_dpm_info_v4_6
// reserved
uint32_t boardreserved[10];
+ );
};
struct atom_smc_dpm_info_v4_7
{
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
// I2C Control
struct smudpm_i2c_controller_config_v2 I2cControllers[8];
@@ -2348,6 +2352,7 @@ struct atom_smc_dpm_info_v4_7
uint8_t Padding8_Psi2;
uint32_t BoardReserved[5];
+ );
};
struct smudpm_i2c_controller_config_v3
@@ -2478,6 +2483,7 @@ struct atom_smc_dpm_info_v4_10
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
// Telemetry Settings
uint16_t GfxMaxCurrent; // in Amps
uint8_t GfxOffset; // in Amps
@@ -2524,6 +2530,7 @@ struct atom_smc_dpm_info_v4_10
uint16_t spare5;
uint32_t reserved[16];
+ );
};
/*
diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
index 43d43d6addc0..8093a98800c3 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
@@ -643,6 +643,7 @@ typedef struct {
// SECTION: BOARD PARAMETERS
// SVI2 Board Parameters
+ struct_group(v4_6,
uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will
request. Multiple steps are taken if voltage change exceeds this value.
uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will
request. Multiple steps are taken if voltage change exceeds this value.
@@ -728,10 +729,10 @@ typedef struct {
uint32_t BoardVoltageCoeffB;// decode by /1000
uint32_t BoardReserved[7];
+ );
// Padding for MMHUB - do not modify this
uint32_t MmHubPadding[8]; // SMU internal use
-
} PPTable_t;
typedef struct {
diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
index 04752ade1016..0b4e6e907e95 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
@@ -725,6 +725,7 @@ typedef struct {
uint32_t Reserved[8];
// SECTION: BOARD PARAMETERS
+ struct_group(v4,
// I2C Control
I2cControllerConfig_t I2cControllers[NUM_I2C_CONTROLLERS];
@@ -809,10 +810,10 @@ typedef struct {
uint8_t Padding8_Loadline;
uint32_t BoardReserved[8];
+ );
[PATCH 20/64] intersil: Use struct_group() for memcpy() region
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
---
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
[PATCH 45/64] intel_th: msu: Use memset_after() for clearing hw header
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
---
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
[PATCH 24/64] staging: wlan-ng: Use struct_group() for memcpy() region
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
---
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
[PATCH 35/64] fortify: Detect struct member overflows in memmove() at compile-time
As done for memcpy(), also update memmove() to use the same tightened
compile-time checks under CONFIG_FORTIFY_SOURCE.
Signed-off-by: Kees Cook
---
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 memmovememmove
-
/* 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 ..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 ..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
[PATCH 52/64] dm integrity: Use struct_group() to zero struct journal_sector
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
---
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
[PATCH 33/64] lib: Introduce CONFIG_TEST_MEMCPY
Before changing anything about memcpy(), memmove(), and memset(), add
run-time tests to check basic behaviors for any regressions.
Signed-off-by: Kees Cook
---
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 ..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+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", d
[PATCH 34/64] fortify: Detect struct member overflows in memcpy() at compile-time
the size and safety of the copy.
For run-time, the "mode 0" size checking and mitigation is left unchanged,
with "mode 1" added only to writes, and only performing a WARN() for
now. This way any missed run-time false positives can be flushed out over
the coming several development cycles, but system builders who have tested
their workloads to be WARN()-free can enable the panic_on_warn=1 sysctl
to immediately gain a mitigation against this class of buffer overflows.
For now, cross-member "mode 1" read detection at compile-time will be
limited to W=1 builds, since it is, unfortunately, very common. As the
priority is solving write overflows, read overflows can be the next
phase. Similarly, run-time cross-member "mode 1" read detection will be
added at a later time, once write false-positives have been handled.
Related classes of flaws that remain unmitigated:
- memcpy() with raw pointers (e.g. void *, char *, etc) have no good
mitigation beyond memory tagging (and even that would only protect
against inter-object overflow, not intra-object neighboring field
overflows). Some kind of "fat pointer" solution is likely needed to
gain proper size-of-buffer awareness.
- type confusion where a higher level type's allocation size does
not match the resulting cast type eventually passed to a deeper
memcpy() call where the compiler cannot see the true type. In
theory, greater static analysis could catch these.
[0] https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html
[1] https://git.kernel.org/linus/6a39e62abbafd1d58d1722f40c7d26ef379c6a2f
Signed-off-by: Kees Cook
---
include/linux/fortify-string.h| 111 --
include/linux/string.h| 5 +-
lib/Makefile | 3 +-
lib/string_helpers.c | 6 +
.../read_overflow2_field-memcpy.c | 5 +
.../write_overflow_field-memcpy.c | 5 +
6 files changed, 120 insertions(+), 15 deletions(-)
create mode 100644 lib/test_fortify/read_overflow2_field-memcpy.c
create mode 100644 lib/test_fortify/write_overflow_field-memcpy.c
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 7e67d02764db..5e79e626172b 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -2,13 +2,17 @@
#ifndef _LINUX_FORTIFY_STRING_H_
#define _LINUX_FORTIFY_STRING_H_
+#include
+
#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
#define __RENAME(x) __asm__(#x)
void fortify_panic(const char *name) __noreturn __cold;
void __read_overflow(void) __compiletime_error("detected read beyond size of
object (1st parameter)");
void __read_overflow2(void) __compiletime_error("detected read beyond size of
object (2nd parameter)");
+void __read_overflow2_field(void) __compiletime_warning("detected read beyond
size of field (2nd parameter); maybe use struct_group()?");
void __write_overflow(void) __compiletime_error("detected write beyond size of
object (1st parameter)");
+void __write_overflow_field(void) __compiletime_warning("detected write beyond
size of field (1st parameter); maybe use struct_group()?");
#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size)
__RENAME(memchr);
@@ -182,22 +186,105 @@ __FORTIFY_INLINE void *memset(void *p, int c,
__kernel_size_t size)
return __underlying_memset(p, c, size);
}
-__FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size)
+/*
+ * To make sure the compiler can enforce protection against buffer overflows,
+ * memcpy(), memmove(), and memset() must not be used beyond individual
+ * struct members. If you need to copy across multiple members, please use
+ * struct_group() to create a named mirror of an anonymous struct union.
+ * (e.g. see struct sk_buff.)
+ *
+ * Mitigation coverage
+ * Bounds checking at:
+ * +---+---+---+---+
+ * | Compile time | Run time |
+ * memcpy() argument sizes:| write | read | write | read |
+ * +---+---+---+---+
+ * memcpy(known, known, constant) | y | y | n/a | n/a |
+ * memcpy(unknown, known, constant) | n | y | V | n/a |
+ * memcpy(known, unknown, constant) | y | n | n/a | V |
+ * memcpy(unknown, unknown, constant) | n | n | V | V |
+ * memcpy(known, known, dynamic) | n | n | b | B |
+ * memcpy(unknown, known, dynamic) | n | n | V | B |
+ * memcpy(known, unknown, dynamic) | n | n | b | V |
+ * memcpy(unknown, unknown, dynamic) | n | n | V
[PATCH 58/64] powerpc: Split memset() to avoid multi-field overflow
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Instead of writing across a field boundary with memset(), move the call
to just the array, and an explicit zeroing of the prior field.
Signed-off-by: Kees Cook
---
drivers/macintosh/smu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/macintosh/smu.c b/drivers/macintosh/smu.c
index 94fb63a7b357..59ce431da7ef 100644
--- a/drivers/macintosh/smu.c
+++ b/drivers/macintosh/smu.c
@@ -848,7 +848,8 @@ int smu_queue_i2c(struct smu_i2c_cmd *cmd)
cmd->read = cmd->info.devaddr & 0x01;
switch(cmd->info.type) {
case SMU_I2C_TRANSFER_SIMPLE:
- memset(&cmd->info.sublen, 0, 4);
+ cmd->info.sublen = 0;
+ memset(&cmd->info.subaddr, 0, 3);
break;
case SMU_I2C_TRANSFER_COMBINED:
cmd->info.devaddr &= 0xfe;
--
2.30.2
[PATCH 49/64] cm4000_cs: Use struct_group() to zero struct cm4000_dev region
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
---
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;
+ );
};
-#defineZERO_DEV(dev) \
- memset(&dev->atr_csum,0,\
- sizeof(struct cm4000_dev) - \
- offsetof(struct cm4000_dev, atr_csum))
+#defineZERO_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
[PATCH 51/64] tracing: Use struct_group() to zero struct trace_iterator
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct trace_iterator that should
be initialized to zero.
Signed-off-by: Kees Cook
---
include/linux/trace_events.h | 26 ++
kernel/trace/trace.c | 4 +---
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index ad413b382a3c..cadad77fe524 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -101,18 +101,20 @@ struct trace_iterator {
boolsnapshot;
/* The below is zeroed out in pipe_read */
- struct trace_seqseq;
- struct trace_entry *ent;
- unsigned long lost_events;
- int leftover;
- int ent_size;
- int cpu;
- u64 ts;
-
- loff_t pos;
- longidx;
-
- /* All new field here will be zeroed out in pipe_read */
+ struct_group(init,
+ struct trace_seqseq;
+ struct trace_entry *ent;
+ unsigned long lost_events;
+ int leftover;
+ int ent_size;
+ int cpu;
+ u64 ts;
+
+ loff_t pos;
+ longidx;
+
+ /* All new field here will be zeroed out in pipe_read */
+ );
};
enum trace_iter_flags {
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c59dd35a6da5..9f83864b0be6 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6691,9 +6691,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
cnt = PAGE_SIZE - 1;
/* reset all but tr, trace, and overruns */
- memset(&iter->seq, 0,
- sizeof(struct trace_iterator) -
- offsetof(struct trace_iterator, seq));
+ memset(&iter->init, 0, sizeof(iter->init));
cpumask_clear(iter->started);
trace_seq_init(&iter->seq);
iter->pos = -1;
--
2.30.2
[PATCH 46/64] IB/mthca: Use memset_after() for clearing mpt_entry
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 --- 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
[PATCH 59/64] fortify: Detect struct member overflows in memset() at compile-time
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
---
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 ..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
[PATCH 48/64] drbd: Use struct_group() to zero algs
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
---
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
Re: [PATCH 31/64] fortify: Explicitly disable Clang support
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 > > --- > > 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
Re: [PATCH 33/64] lib: Introduce CONFIG_TEST_MEMCPY
On Tue, Jul 27, 2021 at 04:31:03PM -0700, 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?
Good point! I will see if that works here; it would make sense to make
this KUnit from the start.
--
Kees Cook
Re: [PATCH 34/64] fortify: Detect struct member overflows in memcpy() at compile-time
On Tue, Jul 27, 2021 at 03:43:27PM -0700, Nick Desaulniers wrote:
> On Tue, Jul 27, 2021 at 2:17 PM Kees Cook wrote:
> >
> > To accelerate the review of potential run-time false positives, it's
> > also worth noting that it is possible to partially automate checking
> > by examining memcpy() buffer argument fields to see if they have
> > a neighboring. It is reasonable to expect that the vast majority of
>
> a neighboring...field?
Whoops, sorry, this should say "array member". I've fixed this to read:
To accelerate the review of potential run-time false positives, it's
also worth noting that it is possible to partially automate checking
by examining the memcpy() buffer argument to check for the destination
struct member having a neighboring array member. It is reasonable to
expect that the vast majority of run-time false positives would look like
the already evaluated and fixed compile-time false positives, where the
most common pattern is neighboring arrays. (And, FWIW, several of the
compile-time fixes were actual bugs.)
> > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> > index 7e67d02764db..5e79e626172b 100644
> > --- a/include/linux/fortify-string.h
> > +++ b/include/linux/fortify-string.h
> > @@ -2,13 +2,17 @@
> > #ifndef _LINUX_FORTIFY_STRING_H_
> > #define _LINUX_FORTIFY_STRING_H_
> >
> > +#include
>
> What are you using from linux/bug.h here?
Thanks; yes, that should have been added in patch 64, when the WARN_ONCE()
use is introduced:
https://lore.kernel.org/lkml/20210727205855.411487-65-keesc...@chromium.org/
> > [...]
> > +#define __fortify_memcpy_chk(p, q, size, p_size, q_size, \
> > +p_size_field, q_size_field, op) ({ \
> > + size_t __fortify_size = (size_t)(size); \
> > + fortify_memcpy_chk(__fortify_size, p_size, q_size, \
> > + p_size_field, q_size_field, #op);\
> > + __underlying_##op(p, q, __fortify_size);\
> > +})
> > +
> > +/*
> > + * __builtin_object_size() must be captured here to avoid evaluating
> > argument
> > + * side-effects further into the macro layers.
> > + */
> > +#define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
> > + __builtin_object_size(p, 0), __builtin_object_size(q, 0), \
> > + __builtin_object_size(p, 1), __builtin_object_size(q, 1), \
> > + memcpy)
>
> Are there other macro expansion sites for `__fortify_memcpy_chk`,
> perhaps later in this series? I don't understand why `memcpy` is
> passed as `func` to `fortify_panic()` rather than continuing to use
> `__func__`?
Yes, memmove() follows exactly the same pattern. Rather than refactoring
the declaration in that patch, this felt cleaner.
https://lore.kernel.org/lkml/20210727205855.411487-36-keesc...@chromium.org/
> > [...]
> > * @count: The number of bytes to copy
> > * @pad: Character to use for padding if space is left in destination.
> > */
> > -static inline void memcpy_and_pad(void *dest, size_t dest_len,
> > - const void *src, size_t count, int pad)
> > +static __always_inline void memcpy_and_pad(void *dest, size_t dest_len,
> > + const void *src, size_t count,
> > + int pad)
>
> Why __always_inline here?
Without it, we run the risk of it being made out of line, and
potentially losing access to the __builtin_object_size() checking of
arguments. Though given some of the Clang bugs, it's possible this needs
to be strictly converted into a macro.
> > [...]
> > #ifdef CONFIG_FORTIFY_SOURCE
> > +/* These are placeholders for fortify compile-time warnings. */
> > +void __read_overflow2_field(void) { }
> > +EXPORT_SYMBOL(__read_overflow2_field);
> > +void __write_overflow_field(void) { }
> > +EXPORT_SYMBOL(__write_overflow_field);
> > +
>
> Don't we rely on these being undefined for Clang to produce a linkage
> failure (until https://reviews.llvm.org/D106030 has landed)? By
> providing a symbol definition we can link against, I don't think
> __compiletime_{warning|error} will warn at all with Clang?
This was intentional because I explicitly do not want to break the build
for new warnings, and there is no way currently for Clang to _warn_
(rather than fail to link). This could be adjusted to break only Clang's
builds, but at this point, it seemed best.
> > [...]
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#define TEST \
> > + memcpy(instance.buf, large, sizeof(instance.buf) + 1)
> > +
> > +#include "test_fortify.h"
> > --
>
> I haven't read the whole series yet, but I assume test_fortify.h was
> provided earlier in the series?
Yup, it's part of the compile-time tests in patch 32:
https://lore.kernel.org/lkml/20210727205855.411487-33-keesc...@chromium.org/
-Kees
--
Kees Cook
Re: [PATCH 01/64] media: omap3isp: Extract struct group for memcpy() region
On Tue, Jul 27, 2021 at 07:55:46PM -0500, Gustavo A. R. Silva wrote:
> On Tue, Jul 27, 2021 at 01:57:52PM -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. Wrap the target region
> > in a common named structure. This additionally fixes a theoretical
> > misalignment of the copy (since the size of "buf" changes between 64-bit
> > and 32-bit, but this is likely never built for 64-bit).
> >
> > FWIW, I think this code is totally broken on 64-bit (which appears to
> > not be a "real" build configuration): it would either always fail (with
> > an uninitialized data->buf_size) or would cause corruption in userspace
> > due to the copy_to_user() in the call path against an uninitialized
> > data->buf value:
> >
> > omap3isp_stat_request_statistics_time32(...)
> > struct omap3isp_stat_data data64;
> > ...
> > omap3isp_stat_request_statistics(stat, &data64);
> >
> > int omap3isp_stat_request_statistics(struct ispstat *stat,
> > struct omap3isp_stat_data *data)
> > ...
> > buf = isp_stat_buf_get(stat, data);
> >
> > static struct ispstat_buffer *isp_stat_buf_get(struct ispstat *stat,
> >struct omap3isp_stat_data
> > *data)
> > ...
> > if (buf->buf_size > data->buf_size) {
> > ...
> > return ERR_PTR(-EINVAL);
> > }
> > ...
> > rval = copy_to_user(data->buf,
> > buf->virt_addr,
> > buf->buf_size);
> >
> > Regardless, additionally initialize data64 to be zero-filled to avoid
> > undefined behavior.
> >
> > Fixes: 378e3f81cb56 ("media: omap3isp: support 64-bit version of
> > omap3isp_stat_data")
> > Signed-off-by: Kees Cook
> > ---
> > drivers/media/platform/omap3isp/ispstat.c | 5 +--
> > include/uapi/linux/omap3isp.h | 44 +--
> > 2 files changed, 36 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/platform/omap3isp/ispstat.c
> > b/drivers/media/platform/omap3isp/ispstat.c
> > index 5b9b57f4d9bf..ea8222fed38e 100644
> > --- a/drivers/media/platform/omap3isp/ispstat.c
> > +++ b/drivers/media/platform/omap3isp/ispstat.c
> > @@ -512,7 +512,7 @@ int omap3isp_stat_request_statistics(struct ispstat
> > *stat,
> > int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
> > struct omap3isp_stat_data_time32 *data)
> > {
> > - struct omap3isp_stat_data data64;
> > + struct omap3isp_stat_data data64 = { };
> > int ret;
> >
> > ret = omap3isp_stat_request_statistics(stat, &data64);
> > @@ -521,7 +521,8 @@ int omap3isp_stat_request_statistics_time32(struct
> > ispstat *stat,
> >
> > data->ts.tv_sec = data64.ts.tv_sec;
> > data->ts.tv_usec = data64.ts.tv_usec;
> > - memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
> > + data->buf = (uintptr_t)data64.buf;
> > + memcpy(&data->frame, &data64.buf, sizeof(data->frame));
>
> I think this should be:
>
> memcpy(..., &data64.frame, ...);
>
> instead.
Whoops; thanks! This is what I get for temporarily silencing the
read-overflow warnings. :)
-Kees
--
Kees Cook
Re: [PATCH 36/64] scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp
On Tue, Jul 27, 2021 at 09:39:39PM -0400, Martin K. Petersen wrote: > > Kees, > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memset(), avoid intentionally writing across > > neighboring fields. > > > > Instead of writing beyond the end of evt_struct->iu.srp.cmd, target the > > upper union (evt_struct->iu.srp) instead, as that's what is being wiped. > > > > Signed-off-by: Kees Cook > > Orthogonal to your change, it wasn't immediately obvious to me that > SRP_MAX_IU_LEN was the correct length to use for an srp_cmd. However, I > traversed the nested unions and it does look OK. Yeah, I had the same fun. Maybe I should add a BUILD_BUG_ON() here to help illustrate the relationship? I did that in a few other places where the equalities weren't very clear. For example, change it to: + BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN); + memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp)); srp_cmd = &evt_struct->iu.srp.cmd; - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > > For good measure I copied Tyrel and Brian. > > Acked-by: Martin K. Petersen For the moment, I'll leave the patch as-is unless you prefer having the BUILD_BUG_ON(). :) Thanks! -Kees > > > --- > > drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c > > b/drivers/scsi/ibmvscsi/ibmvscsi.c > > index e6a3eaaa57d9..7e8beb42d2d3 100644 > > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c > > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c > > @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd > > *cmnd, > > return SCSI_MLQUEUE_HOST_BUSY; > > > > /* Set up the actual SRP IU */ > > + memset(&evt_struct->iu.srp, 0x00, SRP_MAX_IU_LEN); > > srp_cmd = &evt_struct->iu.srp.cmd; > > - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > > srp_cmd->opcode = SRP_CMD; > > memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); > > int_to_scsilun(lun, &srp_cmd->lun); > > -- > Martin K. PetersenOracle Linux Engineering -- Kees Cook
Re: [PATCH 19/64] ip: Use struct_group() for memcpy() regions
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
Re: [PATCH 02/64] mac80211: Use flex-array for radiotap header bitmap
On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote:
> On Tue, Jul 27, 2021 at 01:57: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.
> >
> > The it_present member of struct ieee80211_radiotap_header is treated as a
> > flexible array (multiple u32s can be conditionally present). In order for
> > memcpy() to reason (or really, not reason) about the size of operations
> > against this struct, use of bytes beyond it_present need to be treated
> > as part of the flexible array. Add a union/struct to contain the new
> > "bitmap" member, for use with trailing presence bitmaps and arguments.
> >
> > Additionally improve readability in the iterator code which walks
> > through the bitmaps and arguments.
> >
> > Signed-off-by: Kees Cook
> > ---
> > include/net/ieee80211_radiotap.h | 24
> > net/mac80211/rx.c| 2 +-
> > net/wireless/radiotap.c | 5 ++---
> > 3 files changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/net/ieee80211_radiotap.h
> > b/include/net/ieee80211_radiotap.h
> > index c0854933e24f..101c1e961032 100644
> > --- a/include/net/ieee80211_radiotap.h
> > +++ b/include/net/ieee80211_radiotap.h
> > @@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {
> > */
> > __le16 it_len;
> >
> > - /**
> > -* @it_present: (first) present word
> > -*/
> > - __le32 it_present;
> > + union {
> > + /**
> > +* @it_present: (first) present word
> > +*/
> > + __le32 it_present;
> > +
> > + struct {
> > + /* The compiler makes it difficult to overlap
> > +* a flex-array with an existing singleton,
> > +* so we're forced to add an empty named
> > +* variable here.
> > +*/
> > + struct { } __unused;
> > +
> > + /**
> > +* @bitmap: all presence bitmaps
> > +*/
> > + __le32 bitmap[];
> > + };
> > + };
> > } __packed;
>
> This patch is so confusing...
Yeah, I agree. I tried a few ways, and was unhappy with all of them. :P
>
> Btw, after the end of the __le32 data there is a bunch of other le64,
> u8 and le16 data so the struct is not accurate or complete.
>
> It might be better to re-write this as something like this:
>
> diff --git a/include/net/ieee80211_radiotap.h
> b/include/net/ieee80211_radiotap.h
> index c0854933e24f..0cb5719e9668 100644
> --- a/include/net/ieee80211_radiotap.h
> +++ b/include/net/ieee80211_radiotap.h
> @@ -42,7 +42,10 @@ struct ieee80211_radiotap_header {
> /**
>* @it_present: (first) present word
>*/
> - __le32 it_present;
> + struct {
> + __le32 it_present;
> + char buff[];
> + } data;
> } __packed;
Hm, yes, I can try this. I attempted something similar without the
"only a struct" part; I was trying to avoid the identifier churn, but I
guess seeing it again, it's not _that_ bad. :P
>
> /* version is always 0 */
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 771921c057e8..9cc891364a07 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -328,7 +328,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local
> *local,
>
> rthdr = skb_push(skb, rtap_len);
> memset(rthdr, 0, rtap_len - rtap.len - rtap.pad);
> - it_present = &rthdr->it_present;
> + it_present = (__le32 *)&rthdr->data;
Hm, interesting way to avoid angering the compiler during the later
it_present++ updates. This is subtle ... a passer-by may not understand
why this isn't just "it_present = &rthdr->data.it_present".
I think this is okay with a comment added. I'll give this a spin.
Thanks!
-Kees
>
> /* radiotap header, set always present flags */
> rthdr->it_len = cpu_to_le16(rtap_len);
> @@ -372,7 +372,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local
> *local,
> ieee80211_calculate_rx_timestamp(local, status,
>mpdulen, 0),
> pos);
> - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP
Re: [PATCH 02/64] mac80211: Use flex-array for radiotap header bitmap
On Wed, Jul 28, 2021 at 11:23:23AM +0200, David Sterba wrote: > On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote: > > @@ -372,7 +372,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local > > *local, > > ieee80211_calculate_rx_timestamp(local, status, > > mpdulen, 0), > > pos); > > - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT); > > + rthdr->data.it_present |= cpu_to_le32(1 << > > IEEE80211_RADIOTAP_TSFT); > > A drive-by comment, not related to the patchset, but rather the > ieee80211 driver itself. > > Shift expressions with (1 << NUMBER) can be subtly broken once the > NUMBER is 31 and the value gets silently cast to a 64bit type. It will > become 0xf8000. > > I've checked the IEEE80211_RADIOTAP_* defintions if this is even remotely > possible and yes, IEEE80211_RADIOTAP_EXT == 31. Fortunatelly it seems to > be used with used with a 32bit types (eg. _bitmap_shifter) so there are > no surprises. > > The recommended practice is to always use unsigned types for shifts, so > "1U << ..." at least. Ah, good catch! I think just using BIT() is the right replacement here, yes? I suppose that should be a separate patch. -- Kees Cook
Re: [PATCH 47/64] btrfs: Use memset_after() to clear end of struct
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
> > ---
> > 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
What do you think?
-Kees
--
Kees Cook
Re: [PATCH 04/64] stddef: Introduce struct_group() helper macro
On Wed, Jul 28, 2021 at 12:54:18PM +0200, Rasmus Villemoes wrote:
> On 27/07/2021 22.57, Kees Cook wrote:
>
> > In order to have a regular programmatic way to describe a struct
> > region that can be used for references and sizing, can be examined for
> > bounds checking, avoids forcing the use of intermediate identifiers,
> > and avoids polluting the global namespace, introduce the struct_group()
> > macro. This macro wraps the member declarations to create an anonymous
> > union of an anonymous struct (no intermediate name) and a named struct
> > (for references and sizing):
> >
> > struct foo {
> > int one;
> > struct_group(thing,
> > int two,
> > int three,
> > );
> > int four;
> > };
>
> That example won't compile, the commas after two and three should be
> semicolons.
Oops, yes, thanks. This is why I shouldn't write code that doesn't first
go through a compiler. ;)
> And your implementation relies on MEMBERS not containing any comma
> tokens, but as
>
> int a, b, c, d;
>
> is a valid way to declare multiple members, consider making MEMBERS
> variadic
>
> #define struct_group(NAME, MEMBERS...)
>
> to have it slurp up every subsequent argument and make that work.
Ah! Perfect, thank you. I totally forgot I could do it that way.
>
> >
> > Co-developed-by: Keith Packard
> > Signed-off-by: Keith Packard
> > Signed-off-by: Kees Cook
> > ---
> > include/linux/stddef.h | 34 ++
>
> Bikeshedding a bit, but do we need to add 34 lines that need to be
> preprocessed to virtually each and every translation unit [as opposed to
> adding a struct_group.h header]? Oh well, you need it for struct
> skbuff.h, so it would be pulled in by a lot regardless :(
My instinct is to make these kinds of helpers "always available" (like
sizeof_field(), etc), but I have no strong opinion on where it should
live. If the consensus is to move it, I certainly can! :)
-Kees
--
Kees Cook
Re: [PATCH 02/64] mac80211: Use flex-array for radiotap header bitmap
On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote:
> On Tue, Jul 27, 2021 at 01:57: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.
> >
> > The it_present member of struct ieee80211_radiotap_header is treated as a
> > flexible array (multiple u32s can be conditionally present). In order for
> > memcpy() to reason (or really, not reason) about the size of operations
> > against this struct, use of bytes beyond it_present need to be treated
> > as part of the flexible array. Add a union/struct to contain the new
> > "bitmap" member, for use with trailing presence bitmaps and arguments.
> >
> > Additionally improve readability in the iterator code which walks
> > through the bitmaps and arguments.
> >
> > Signed-off-by: Kees Cook
> > ---
> > include/net/ieee80211_radiotap.h | 24
> > net/mac80211/rx.c| 2 +-
> > net/wireless/radiotap.c | 5 ++---
> > 3 files changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/net/ieee80211_radiotap.h
> > b/include/net/ieee80211_radiotap.h
> > index c0854933e24f..101c1e961032 100644
> > --- a/include/net/ieee80211_radiotap.h
> > +++ b/include/net/ieee80211_radiotap.h
> > @@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {
> > */
> > __le16 it_len;
> >
> > - /**
> > -* @it_present: (first) present word
> > -*/
> > - __le32 it_present;
> > + union {
> > + /**
> > +* @it_present: (first) present word
> > +*/
> > + __le32 it_present;
> > +
> > + struct {
> > + /* The compiler makes it difficult to overlap
> > +* a flex-array with an existing singleton,
> > +* so we're forced to add an empty named
> > +* variable here.
> > +*/
> > + struct { } __unused;
> > +
> > + /**
> > +* @bitmap: all presence bitmaps
> > +*/
> > + __le32 bitmap[];
> > + };
> > + };
> > } __packed;
>
> This patch is so confusing...
Right, unfortunately your patch doesn't work under the strict memcpy().
:(
Here are the constraints I navigated to come to the original patch I
sent:
* I need to directly reference a flexible array for the it_present
pointer because pos is based on it, and the compiler thinks pos
walks off the end of the struct:
In function 'fortify_memcpy_chk',
inlined from 'ieee80211_add_rx_radiotap_header' at
net/mac80211/rx.c:652:3:
./include/linux/fortify-string.h:285:4: warning: call to
'__write_overflow_field' declared with attribute warning: detected write beyond
size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
285 |__write_overflow_field();
|^~~~
* It's churn/fragile to change the sizeof(), so I can't just do:
- __le32 it_present;
+ __le32 it_bitmap[];
* I want to use a union:
- __le32 it_present;
+ union {
+ __le32 it_present;
+ __le32 it_bitmap[];
+ };
* ... but I can't actually use a union because of compiler constraints
on flexible array members:
./include/net/ieee80211_radiotap.h:50:10: error: flexible array member
in union
50 | __le32 it_optional[];
| ^~~
* So I came to the horrible thing I original sent. :P
If I could escape the __le32 *it_present incrementing, I could use a
simple change:
__le32 it_present;
+ __le32 it_optional[];
> Btw, after the end of the __le32 data there is a bunch of other le64,
> u8 and le16 data so the struct is not accurate or complete.
Hm, docs seem to indicate that the packet format is multiples of u32?
*shrug*
Hmpf.
-Kees
--
Kees Cook
Re: [PATCH 02/64] mac80211: Use flex-array for radiotap header bitmap
On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote:
> On Tue, Jul 27, 2021 at 01:57:53PM -0700, Kees Cook wrote:
> > [...]
> > - /**
> > -* @it_present: (first) present word
> > -*/
> > - __le32 it_present;
> > + union {
> > + /**
> > +* @it_present: (first) present word
> > +*/
> > + __le32 it_present;
> > +
> > + struct {
> > + /* The compiler makes it difficult to overlap
> > +* a flex-array with an existing singleton,
> > +* so we're forced to add an empty named
> > +* variable here.
> > +*/
> > + struct { } __unused;
> > +
> > + /**
> > +* @bitmap: all presence bitmaps
> > +*/
> > + __le32 bitmap[];
> > + };
> > + };
> > } __packed;
>
> This patch is so confusing...
>
> Btw, after the end of the __le32 data there is a bunch of other le64,
> u8 and le16 data so the struct is not accurate or complete.
>
> It might be better to re-write this as something like this:
>
> diff --git a/include/net/ieee80211_radiotap.h
> b/include/net/ieee80211_radiotap.h
> index c0854933e24f..0cb5719e9668 100644
> --- a/include/net/ieee80211_radiotap.h
> +++ b/include/net/ieee80211_radiotap.h
> @@ -42,7 +42,10 @@ struct ieee80211_radiotap_header {
> /**
>* @it_present: (first) present word
>*/
> - __le32 it_present;
> + struct {
> + __le32 it_present;
> + char buff[];
> + } data;
> } __packed;
Ah-ha, got it:
diff --git a/include/net/ieee80211_radiotap.h b/include/net/ieee80211_radiotap.h
index c0854933e24f..6b7274edb3c6 100644
--- a/include/net/ieee80211_radiotap.h
+++ b/include/net/ieee80211_radiotap.h
@@ -43,6 +43,10 @@ struct ieee80211_radiotap_header {
* @it_present: (first) present word
*/
__le32 it_present;
+ /**
+* @it_optional: all remaining presence bitmaps
+*/
+ __le32 it_optional[];
} __packed;
/* version is always 0 */
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 2563473b5cf1..b6a960d37278 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -359,7 +359,13 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local
*local,
put_unaligned_le32(it_present_val, it_present);
- pos = (void *)(it_present + 1);
+ /*
+* This references through an offset into it_optional[] rather
+* than via it_present otherwise later uses of pos will cause
+* the compiler to think we have walked past the end of the
+* struct member.
+*/
+ pos = (void *)&rthdr->it_optional[it_present - rthdr->it_optional];
/* the order of the following fields is important */
diff --git a/net/wireless/radiotap.c b/net/wireless/radiotap.c
index 36f1b59a78bf..081f0a3bdfe1 100644
--- a/net/wireless/radiotap.c
+++ b/net/wireless/radiotap.c
@@ -115,10 +115,9 @@ int ieee80211_radiotap_iterator_init(
iterator->_max_length = get_unaligned_le16(&radiotap_header->it_len);
iterator->_arg_index = 0;
iterator->_bitmap_shifter =
get_unaligned_le32(&radiotap_header->it_present);
- iterator->_arg = (uint8_t *)radiotap_header + sizeof(*radiotap_header);
+ iterator->_arg = (uint8_t *)radiotap_header->it_optional;
iterator->_reset_on_ext = 0;
- iterator->_next_bitmap = &radiotap_header->it_present;
- iterator->_next_bitmap++;
+ iterator->_next_bitmap = radiotap_header->it_optional;
iterator->_vns = vns;
iterator->current_namespace = &radiotap_ns;
iterator->is_radiotap_ns = 1;
--
Kees Cook
Re: [PATCH 62/64] netlink: Avoid false-positive memcpy() warning
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
> >> ---
> >> 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 */
> >> + __u8contents[];
> >
> > 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
Re: [PATCH 62/64] netlink: Avoid false-positive memcpy() warning
On Wed, Jul 28, 2021 at 07:49:46AM +0200, 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
> > ---
> > 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 */
> > + __u8contents[];
>
> Is this ok to change a public, userspace visable, structure?
>
> Nothing breaks?
It really shouldn't break anything. Adding a flex array doesn't change
the size. And with Rasmus's suggestion (naming it "nlmsg_content") it
should be safe against weird global macro collisions, etc.
--
Kees Cook
Re: [PATCH 48/64] drbd: Use struct_group() to zero algs
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
> > ---
> > 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
Re: [PATCH 34/64] fortify: Detect struct member overflows in memcpy() at compile-time
On Wed, Jul 28, 2021 at 01:19:59PM +0200, Rasmus Villemoes wrote:
> On 27/07/2021 22.58, Kees Cook wrote:
>
> > At its core, FORTIFY_SOURCE uses the compiler's __builtin_object_size()
> > internal[0] to determine the available size at a target address based on
> > the compile-time known structure layout details. It operates in two
> > modes: outer bounds (0) and inner bounds (1). In mode 0, the size of the
> > enclosing structure is used. In mode 1, the size of the specific field
> > is used. For example:
> >
> > struct object {
> > u16 scalar1;/* 2 bytes */
> > char array[6]; /* 6 bytes */
> > u64 scalar2;/* 8 bytes */
> > u32 scalar3;/* 4 bytes */
> > } instance;
> >
> >
> > __builtin_object_size(instance.array, 0) == 18, since the remaining size
> > of the enclosing structure starting from "array" is 18 bytes (6 + 8 + 4).
>
> I think the compiler would usually end up making that struct size 24,
> with 4 bytes of trailing padding (at least when alignof(u64) is 8). In
> that case, does __builtin_object_size(instance.array, 0) actually
> evaluate to 18, or to 22? A quick test on x86-64 suggests the latter, so
> the memcpy(, , 20) would not be a violation.
>
> Perhaps it's better to base the example on something which doesn't have
> potential trailing padding - so either add another 4 byte member, or
> also make scalar2 u32.
Yup, totally right. Thanks! I've fixed the example now for v2.
--
Kees Cook
Re: [PATCH 01/64] media: omap3isp: Extract struct group for memcpy() region
_hole_dynamic_partial ok
But when _all_ members are initialized, the padding is _not_:
stackinit: small_hole_static_all FAIL (uninit bytes: 3)
stackinit: big_hole_static_all FAIL (uninit bytes: 124)
stackinit: trailing_hole_static_all FAIL (uninit bytes: 7)
stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3)
stackinit: big_hole_dynamic_all FAIL (uninit bytes: 124)
stackinit: trailing_hole_dynamic_all FAIL (uninit bytes: 7)
As expected, assigning to members outside of initialization leaves
padding uninitialized:
stackinit: small_hole_runtime_partial FAIL (uninit bytes: 23)
stackinit: big_hole_runtime_partial FAIL (uninit bytes: 127)
stackinit: trailing_hole_runtime_partial FAIL (uninit bytes: 24)
stackinit: small_hole_runtime_all FAIL (uninit bytes: 3)
stackinit: big_hole_runtime_all FAIL (uninit bytes: 124)
stackinit: trailing_hole_runtime_all FAIL (uninit bytes: 7)
> What doesn't initialize struct holes is assignments:
>
> struct foo foo = *bar;
Right. Object to object assignments do not clear padding:
stackinit: small_hole_assigned_copy XFAIL (uninit bytes: 3)
stackinit: big_hole_assigned_copy XFAIL (uninit bytes: 124)
stackinit: trailing_hole_assigned_copy XFAIL (uninit bytes: 7)
And whole-object assignments of cast initializers follow the pattern of
basic initializers, which makes sense given the behavior of initializers
and direct assignment tests above. e.g.:
obj = (type){ .member = ... };
stackinit: small_hole_assigned_static_partial ok
stackinit: small_hole_assigned_dynamic_partial ok
stackinit: big_hole_assigned_dynamic_partial ok
stackinit: big_hole_assigned_static_partial ok
stackinit: trailing_hole_assigned_dynamic_partial ok
stackinit: trailing_hole_assigned_static_partial ok
stackinit: small_hole_assigned_static_all FAIL (uninit bytes: 3)
stackinit: small_hole_assigned_dynamic_all FAIL (uninit bytes: 3)
stackinit: big_hole_assigned_static_all FAIL (uninit bytes: 124)
stackinit: big_hole_assigned_dynamic_all FAIL (uninit bytes: 124)
stackinit: trailing_hole_assigned_dynamic_all FAIL (uninit bytes: 7)
stackinit: trailing_hole_assigned_static_all FAIL (uninit bytes: 7)
So, yeah, it's not very stable.
-Kees
[1] https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576341.html
--
Kees Cook
Re: [PATCH 02/64] mac80211: Use flex-array for radiotap header bitmap
On Thu, Jul 29, 2021 at 12:45:47PM +0200, David Sterba wrote:
> On Wed, Jul 28, 2021 at 02:54:52PM -0700, Kees Cook wrote:
> > On Wed, Jul 28, 2021 at 11:23:23AM +0200, David Sterba wrote:
> > > On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote:
> > > > @@ -372,7 +372,7 @@ ieee80211_add_rx_radiotap_header(struct
> > > > ieee80211_local *local,
> > > > ieee80211_calculate_rx_timestamp(local, status,
> > > > mpdulen, 0),
> > > > pos);
> > > > - rthdr->it_present |= cpu_to_le32(1 <<
> > > > IEEE80211_RADIOTAP_TSFT);
> > > > + rthdr->data.it_present |= cpu_to_le32(1 <<
> > > > IEEE80211_RADIOTAP_TSFT);
> > >
> > > A drive-by comment, not related to the patchset, but rather the
> > > ieee80211 driver itself.
> > >
> > > Shift expressions with (1 << NUMBER) can be subtly broken once the
> > > NUMBER is 31 and the value gets silently cast to a 64bit type. It will
> > > become 0xf8000.
> > >
> > > I've checked the IEEE80211_RADIOTAP_* defintions if this is even remotely
> > > possible and yes, IEEE80211_RADIOTAP_EXT == 31. Fortunatelly it seems to
> > > be used with used with a 32bit types (eg. _bitmap_shifter) so there are
> > > no surprises.
> > >
> > > The recommended practice is to always use unsigned types for shifts, so
> > > "1U << ..." at least.
> >
> > Ah, good catch! I think just using BIT() is the right replacement here,
> > yes? I suppose that should be a separate patch.
>
> I found definition of BIT in vdso/bits.h, that does not sound like a
> standard header, besides that it shifts 1UL, that may not be necessary
> everywhere. IIRC there were objections against using the macro at all.
3945ff37d2f4 ("linux/bits.h: Extract common header for vDSO") moved it
there from linux/bits.h, and linux/bits.h now includes vdso/bits.h, so
it is still ever-present. :)
--
Kees Cook
Re: [PATCH 01/64] media: omap3isp: Extract struct group for memcpy() region
On Fri, Jul 30, 2021 at 12:00:54PM +0300, Dan Carpenter wrote:
> On Fri, Jul 30, 2021 at 10:38:45AM +0200, David Sterba wrote:
> > Then is explicit memset the only reliable way accross all compiler
> > flavors and supported versions?
> >
>
> The = { } initializer works. It's only when you start partially
> initializing the struct that it doesn't initialize holes.
No, partial works. It's when you _fully_ initialize the struct where the
padding doesn't get initialized. *sob*
struct foo {
u8 flag;
/* padding */
void *ptr;
};
These are fine:
struct foo ok1 = { };
struct foo ok2 = { .flag = 7 };
struct foo ok3 = { .ptr = NULL };
This is not:
struct foo bad = { .flag = 7, .ptr = NULL };
(But, of course, it depends on padding size, compiler version, and
architecture. i.e. things remain unreliable.)
--
Kees Cook
Re: [PATCH 01/64] media: omap3isp: Extract struct group for memcpy() region
On Fri, Jul 30, 2021 at 10:08:03AM -0700, Nick Desaulniers wrote:
> On Fri, Jul 30, 2021 at 9:44 AM Kees Cook wrote:
> >
> > On Fri, Jul 30, 2021 at 12:00:54PM +0300, Dan Carpenter wrote:
> > > On Fri, Jul 30, 2021 at 10:38:45AM +0200, David Sterba wrote:
> > > > Then is explicit memset the only reliable way accross all compiler
> > > > flavors and supported versions?
> > > >
> > >
> > > The = { } initializer works. It's only when you start partially
> > > initializing the struct that it doesn't initialize holes.
> >
> > No, partial works. It's when you _fully_ initialize the struct where the
> > padding doesn't get initialized. *sob*
>
> I'm pretty sure that this has more to do with whether or not the
> compiler applies SROA then observes uses of the individual members or
> not.
Ultimately, it's just not consistent, so thank goodness for
-ftrivial-auto-var-init=zero. :)
--
Kees Cook
Re: [PATCH 04/64] stddef: Introduce struct_group() helper macro
On Fri, Jul 30, 2021 at 10:19:20PM +, Williams, Dan J wrote:
> On Wed, 2021-07-28 at 14:59 -0700, Kees Cook wrote:
> > On Wed, Jul 28, 2021 at 12:54:18PM +0200, Rasmus Villemoes wrote:
> > > On 27/07/2021 22.57, Kees Cook wrote:
> > >
> > > > In order to have a regular programmatic way to describe a struct
> > > > region that can be used for references and sizing, can be examined for
> > > > bounds checking, avoids forcing the use of intermediate identifiers,
> > > > and avoids polluting the global namespace, introduce the struct_group()
> > > > macro. This macro wraps the member declarations to create an anonymous
> > > > union of an anonymous struct (no intermediate name) and a named struct
> > > > (for references and sizing):
> > > >
> > > > struct foo {
> > > > int one;
> > > > struct_group(thing,
> > > > int two,
> > > > int three,
> > > > );
> > > > int four;
> > > > };
> > >
> > > That example won't compile, the commas after two and three should be
> > > semicolons.
> >
> > Oops, yes, thanks. This is why I shouldn't write code that doesn't first
> > go through a compiler. ;)
> >
> > > And your implementation relies on MEMBERS not containing any comma
> > > tokens, but as
> > >
> > > int a, b, c, d;
> > >
> > > is a valid way to declare multiple members, consider making MEMBERS
> > > variadic
> > >
> > > #define struct_group(NAME, MEMBERS...)
> > >
> > > to have it slurp up every subsequent argument and make that work.
> >
> > Ah! Perfect, thank you. I totally forgot I could do it that way.
>
> This is great Kees. It just so happens it would clean-up what we are
> already doing in drivers/cxl/cxl.h for anonymous + named register block
> pointers. However in the cxl case it also needs the named structure to
> be typed. Any appetite for a typed version of this?
Oh cool! Yeah, totally I can expand it. Thanks for the suggestion!
>
> Here is a rough idea of the cleanup it would induce in drivers/cxl/:
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 53927f9fa77e..a2308c995654 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -75,52 +75,19 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr)
> #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
> #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
>
> -#define CXL_COMPONENT_REGS() \
> - void __iomem *hdm_decoder
> -
> -#define CXL_DEVICE_REGS() \
> - void __iomem *status; \
> - void __iomem *mbox; \
> - void __iomem *memdev
> -
> -/* See note for 'struct cxl_regs' for the rationale of this organization */
> /*
> - * CXL_COMPONENT_REGS - Common set of CXL Component register block base
> pointers
> * @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
> - */
> -struct cxl_component_regs {
> - CXL_COMPONENT_REGS();
> -};
> -
> -/* See note for 'struct cxl_regs' for the rationale of this organization */
> -/*
> - * CXL_DEVICE_REGS - Common set of CXL Device register block base pointers
> * @status: CXL 2.0 8.2.8.3 Device Status Registers
> * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
> * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
> */
> -struct cxl_device_regs {
> - CXL_DEVICE_REGS();
> -};
> -
> -/*
> - * Note, the anonymous union organization allows for per
> - * register-block-type helper routines, without requiring block-type
> - * agnostic code to include the prefix.
> - */
> struct cxl_regs {
> - union {
> - struct {
> - CXL_COMPONENT_REGS();
> - };
> - struct cxl_component_regs component;
> - };
> - union {
> - struct {
> - CXL_DEVICE_REGS();
> - };
> - struct cxl_device_regs device_regs;
> - };
> + struct_group_typed(cxl_component_regs, component,
> + void __iomem *hdm_decoder;
> + );
> + struct_group_typed(cxl_device_regs, device_regs,
> + void __iomem *status, *mbox, *memdev;
> + );
> };
>
> struct cxl_reg_map {
> diff --git a/include/linux/stddef.h b/include/linux/stddef.h
> index cf7f866944f9..84b7de24ffb5 100644
> --- a/include/linux/stddef.h
> +++ b/include/linux/stddef
Re: [PATCH 25/64] drm/mga/mga_ioc32: Use struct_group() for memcpy() region
On Thu, Jul 29, 2021 at 02:11:27PM +0200, Daniel Vetter wrote: > On Wed, Jul 28, 2021 at 07:56:40AM +0200, Greg Kroah-Hartman wrote: > > On Tue, Jul 27, 2021 at 01:58:16PM -0700, Kees Cook wrote: > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > > field bounds checking for memcpy(), memmove(), and memset(), avoid > > > intentionally writing across neighboring fields. > > > > > > Use struct_group() in struct drm32_mga_init around members chipset, sgram, > > > maccess, fb_cpp, front_offset, front_pitch, back_offset, back_pitch, > > > depth_cpp, depth_offset, depth_pitch, texture_offset, and texture_size, > > > so they can be referenced together. This will allow memcpy() and sizeof() > > > to more easily reason about sizes, improve readability, and avoid future > > > warnings about writing beyond the end of chipset. > > > > > > "pahole" shows no size nor member offset changes to struct drm32_mga_init. > > > "objdump -d" shows no meaningful object code changes (i.e. only source > > > line number induced differences and optimizations). > > > > > > Note that since this includes a UAPI header, struct_group() has been > > > explicitly redefined local to the header. > > > [...] > > > > Why can you use __struct_group in this uapi header, but not the > > networking one? > > If there's others, maybe we can stuff the uapi __struct_group into > linux/types.h where all the other __ uapi types hang out? Ah yeah; it looks like include/uapi/linux/stddef.h is the place for it. > Anyway mga is very dead, I don't anyone cares. > > Acked-by: Daniel Vetter > > I'm assuming this goes in through a topic pull from you? Thanks! Yeah, my intention is to carry this as topic branch for Linus. -Kees -- Kees Cook
Re: [PATCH 54/64] ipv6: Use struct_group() to zero rt6_info
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
Re: [PATCH 04/64] stddef: Introduce struct_group() helper macro
On Sat, Jul 31, 2021 at 07:24:44AM +0200, Rasmus Villemoes wrote:
> On Sat, Jul 31, 2021, 04:59 Kees Cook wrote:
>
> > On Fri, Jul 30, 2021 at 10:19:20PM +, Williams, Dan J wrote:
> > > On Wed, 2021-07-28 at 14:59 -0700, Kees Cook wrote:
> >
> > > /**
> > > * struct_group(NAME, MEMBERS)
> > > *
> > > @@ -67,7 +73,10 @@ enum {
> > > * @NAME: The name of the mirrored sub-struct
> > > * @MEMBERS: The member declarations for the mirrored structs
> > > */
> > > -#define struct_group(NAME, MEMBERS)\
> > > +#define struct_group(NAME, MEMBERS...) \
> > > struct_group_attr(NAME, /* no attrs */, MEMBERS)
> > >
> > > +#define struct_group_typed(TYPE, NAME, MEMBERS...) \
> > > + struct_group_attr_typed(TYPE, NAME, /* no attrs */, MEMBERS)
> > > +
> > > #endif
> >
> > Awesome! My instinct is to expose the resulting API as:
> >
> > __struct_group(type, name, attrs, members...)
> >
> > struct_group(name, members...)
> > struct_group_attr(name, attrs, members...)
> > struct_group_typed(type, name, members...)
>
> Bikeshed: can we use proper nomenclature please. s/type/tag/,
> s/typed/tagged.
Ah! Thank you. I went looking for the spec on what these are called and
couldn't find it. "struct $tag" is the type, then, yes? So IIUC now:
|type | members | name
| tag
struct foo { int bar; } baz;
--
Kees Cook
Re: [PATCH 47/64] btrfs: Use memset_after() to clear end of struct
On Thu, Jul 29, 2021 at 12:33:37PM +0200, David Sterba wrote:
> 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
> > > > ---
> > > > 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?
For v2, I bikeshed this to "memset_startat" since "from" is semantically
close to "source" which I thought might be confusing. (I, too, did not
like "starting".) :)
Can I make "bikeshed" a verb? :P
--
Kees Cook
Re: [PATCH 39/64] mac80211: Use memset_after() to clear tx status
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 > --- > 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
Re: [PATCH 42/64] net: qede: Use memset_after() for counters
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
> ---
> 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 mutexqede_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
Re: [PATCH 42/64] net: qede: Use memset_after() for counters
On Mon, Aug 02, 2021 at 02:29:28PM +, 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
> > > ---
> > > 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 mutexqede_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
[PATCH 2/2] drm/radeon: Avoid power table parsing memory leaks
Avoid leaving a hanging pre-allocated clock_info if last mode is
invalid, and avoid heap corruption if no valid modes are found.
Fixes: 6991b8f2a319 ("drm/radeon/kms: fix segfault in pm rework")
Signed-off-by: Kees Cook
---
drivers/gpu/drm/radeon/radeon_atombios.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c
b/drivers/gpu/drm/radeon/radeon_atombios.c
index f9f4efa1738c..28c4413f4dc8 100644
--- a/drivers/gpu/drm/radeon/radeon_atombios.c
+++ b/drivers/gpu/drm/radeon/radeon_atombios.c
@@ -2120,11 +2120,14 @@ static int radeon_atombios_parse_power_table_1_3(struct
radeon_device *rdev)
return state_index;
/* last mode is usually default, array is low to high */
for (i = 0; i < num_modes; i++) {
- rdev->pm.power_state[state_index].clock_info =
- kcalloc(1, sizeof(struct radeon_pm_clock_info),
- GFP_KERNEL);
+ /* avoid memory leaks from invalid modes or unknown frev. */
+ if (!rdev->pm.power_state[state_index].clock_info) {
+ rdev->pm.power_state[state_index].clock_info =
+ kzalloc(sizeof(struct radeon_pm_clock_info),
+ GFP_KERNEL);
+ }
if (!rdev->pm.power_state[state_index].clock_info)
- return state_index;
+ goto out;
rdev->pm.power_state[state_index].num_clock_modes = 1;
rdev->pm.power_state[state_index].clock_info[0].voltage.type =
VOLTAGE_NONE;
switch (frev) {
@@ -2243,8 +2246,15 @@ static int radeon_atombios_parse_power_table_1_3(struct
radeon_device *rdev)
break;
}
}
+out:
+ /* free any unused clock_info allocation. */
+ if (state_index && state_index < num_modes) {
+ kfree(rdev->pm.power_state[state_index].clock_info);
+ rdev->pm.power_state[state_index].clock_info = NULL;
+ }
+
/* last mode is usually default */
- if (rdev->pm.default_power_state_index == -1) {
+ if (state_index && rdev->pm.default_power_state_index == -1) {
rdev->pm.power_state[state_index - 1].type =
POWER_STATE_TYPE_DEFAULT;
rdev->pm.default_power_state_index = state_index - 1;
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/radeon: Fix off-by-one power_state index heap overwrite
An out of bounds write happens when setting the default power state.
KASAN sees this as:
[drm] radeon: 512M of GTT memory ready.
[drm] GART: num cpu pages 131072, num gpu pages 131072
==
BUG: KASAN: slab-out-of-bounds in
radeon_atombios_parse_power_table_1_3+0x1837/0x1998 [radeon]
Write of size 4 at addr 88810178d858 by task systemd-udevd/157
CPU: 0 PID: 157 Comm: systemd-udevd Not tainted 5.12.0-E620 #50
Hardware name: eMachineseMachines E620 /Nile , BIOS V1.03
09/30/2008
Call Trace:
dump_stack+0xa5/0xe6
print_address_description.constprop.0+0x18/0x239
kasan_report+0x170/0x1a8
radeon_atombios_parse_power_table_1_3+0x1837/0x1998 [radeon]
radeon_atombios_get_power_modes+0x144/0x1888 [radeon]
radeon_pm_init+0x1019/0x1904 [radeon]
rs690_init+0x76e/0x84a [radeon]
radeon_device_init+0x1c1a/0x21e5 [radeon]
radeon_driver_load_kms+0xf5/0x30b [radeon]
drm_dev_register+0x255/0x4a0 [drm]
radeon_pci_probe+0x246/0x2f6 [radeon]
pci_device_probe+0x1aa/0x294
really_probe+0x30e/0x850
driver_probe_device+0xe6/0x135
device_driver_attach+0xc1/0xf8
__driver_attach+0x13f/0x146
bus_for_each_dev+0xfa/0x146
bus_add_driver+0x2b3/0x447
driver_register+0x242/0x2c1
do_one_initcall+0x149/0x2fd
do_init_module+0x1ae/0x573
load_module+0x4dee/0x5cca
__do_sys_finit_module+0xf1/0x140
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xae
Without KASAN, this will manifest later when the kernel attempts to
allocate memory that was stomped, since it collides with the inline slab
freelist pointer:
invalid opcode: [#1] SMP NOPTI
CPU: 0 PID: 781 Comm: openrc-run.sh Tainted: GW 5.10.12-gentoo-E620 #2
Hardware name: eMachineseMachines E620 /Nile , BIOS V1.03
09/30/2008
RIP: 0010:kfree+0x115/0x230
Code: 89 c5 e8 75 ea ff ff 48 8b 00 0f ba e0 09 72 63 e8 1f f4 ff ff 41 89 c4
48 8b 45 00 0f ba e0 10 72 0a 48 8b 45 08 a8 01 75 02 <0f> 0b 44 89 e1 48 c7 c2
00 f0 ff ff be 06 00 00 00 48 d3 e2 48 c7
RSP: 0018:b42f40267e10 EFLAGS: 00010246
RAX: d61280ee8d88 RBX: 0004 RCX: 801d
RDX: 4000 RSI: ba1360b0 RDI: d61280ee8d80
RBP: d61280ee8d80 R08: b91bebdf R09:
R10: 8fe2c1047ac8 R11: R12:
R13: R14: R15: 0100
FS: 7fe80eff6b68() GS:8fe339c0() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7fe80eec7bc0 CR3: 38012000 CR4: 06f0
Call Trace:
__free_fdtable+0x16/0x1f
put_files_struct+0x81/0x9b
do_exit+0x433/0x94d
do_group_exit+0xa6/0xa6
__x64_sys_exit_group+0xf/0xf
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fe80ef64bea
Code: Unable to access opcode bytes at RIP 0x7fe80ef64bc0.
RSP: 002b:7ffdb1c47528 EFLAGS: 0246 ORIG_RAX: 00e7
RAX: ffda RBX: 0003 RCX: 7fe80ef64bea
RDX: 7fe80ef64f60 RSI: RDI:
RBP: R08: 0001 R09:
R10: 7fe80ee2c620 R11: 0246 R12: 7fe80eff41e0
R13: R14: 0024 R15: 7fe80edf9cd0
Modules linked in: radeon(+) ath5k(+) snd_hda_codec_realtek ...
Use a valid power_state index when initializing the "flags" and "misc"
and "misc2" fields.
Reported-by: Erhard F.
Fixes: a48b9b4edb8b ("drm/radeon/kms/pm: add asic specific callbacks for
getting power state (v2)")
Fixes: 79daedc94281 ("drm/radeon/kms: minor pm cleanups")
Signed-off-by: Kees Cook
---
drivers/gpu/drm/radeon/radeon_atombios.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c
b/drivers/gpu/drm/radeon/radeon_atombios.c
index 42301b4e56f5..f9f4efa1738c 100644
--- a/drivers/gpu/drm/radeon/radeon_atombios.c
+++ b/drivers/gpu/drm/radeon/radeon_atombios.c
@@ -2250,10 +2250,10 @@ static int radeon_atombios_parse_power_table_1_3(struct
radeon_device *rdev)
rdev->pm.default_power_state_index = state_index - 1;
rdev->pm.power_state[state_index - 1].default_clock_mode =
&rdev->pm.power_state[state_index - 1].clock_info[0];
- rdev->pm.power_state[state_index].flags &=
+ rdev->pm.power_state[state_index - 1].flags &=
~RADEON_PM_STATE_SINGLE_DISPLAY_ONLY;
- rdev->pm.power_state[state_index].misc = 0;
- rdev->pm.power_state[state_index].misc2 = 0;
+ rdev->pm.power_state[state_index - 1].misc = 0;
+ rdev->pm.power_state[state_index - 1].misc2 = 0;
}
return state_index;
}
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] drm/radeon: Fix off-by-one power_state index heap overwrite
Hi, This is an attempt at fixing a bug[1] uncovered by the relocation of the slab freelist pointer offset, as well as some related clean-ups. I don't have hardware to do runtime testing, but it builds. ;) -Kees [1] https://bugzilla.kernel.org/show_bug.cgi?id=211537 Kees Cook (2): drm/radeon: Fix off-by-one power_state index heap overwrite drm/radeon: Avoid power table parsing memory leaks drivers/gpu/drm/radeon/radeon_atombios.c | 26 1 file changed, 18 insertions(+), 8 deletions(-) -- 2.25.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 10/64] lib80211: Use struct_group() for memcpy() region
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
Re: [PATCH 39/64] mac80211: Use memset_after() to clear tx status
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
[PATCH] drm/i915: Use designated initializers for init/exit table
The kernel builds with -Werror=designated-init, and __designated_init
is used by CONFIG_GCC_PLUGIN_RANDSTRUCT for automatically selected (all
function pointer) structures. Include the field names in the init/exit
table. Avoids warnings like:
drivers/gpu/drm/i915/i915_module.c:59:4: error: positional initialization of
field in 'struct' declared with 'designated_init' attribute
[-Werror=designated-init]
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: David Airlie
Cc: intel-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Fixes: a04ea6ae7c67 ("drm/i915: Use a table for i915_init/exit (v2)")
Signed-off-by: Kees Cook
---
drivers/gpu/drm/i915/i915_module.c | 37 +++---
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index c578ea8f56a0..d8b4482c69d0 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -47,19 +47,30 @@ static const struct {
int (*init)(void);
void (*exit)(void);
} init_funcs[] = {
- { i915_check_nomodeset, NULL },
- { i915_active_module_init, i915_active_module_exit },
- { i915_buddy_module_init, i915_buddy_module_exit },
- { i915_context_module_init, i915_context_module_exit },
- { i915_gem_context_module_init, i915_gem_context_module_exit },
- { i915_objects_module_init, i915_objects_module_exit },
- { i915_request_module_init, i915_request_module_exit },
- { i915_scheduler_module_init, i915_scheduler_module_exit },
- { i915_vma_module_init, i915_vma_module_exit },
- { i915_mock_selftests, NULL },
- { i915_pmu_init, i915_pmu_exit },
- { i915_register_pci_driver, i915_unregister_pci_driver },
- { i915_perf_sysctl_register, i915_perf_sysctl_unregister },
+ { .init = i915_check_nomodeset },
+ { .init = i915_active_module_init,
+ .exit = i915_active_module_exit },
+ { .init = i915_buddy_module_init,
+ .exit = i915_buddy_module_exit },
+ { .init = i915_context_module_init,
+ .exit = i915_context_module_exit },
+ { .init = i915_gem_context_module_init,
+ .exit = i915_gem_context_module_exit },
+ { .init = i915_objects_module_init,
+ .exit = i915_objects_module_exit },
+ { .init = i915_request_module_init,
+ .exit = i915_request_module_exit },
+ { .init = i915_scheduler_module_init,
+ .exit = i915_scheduler_module_exit },
+ { .init = i915_vma_module_init,
+ .exit = i915_vma_module_exit },
+ { .init = i915_mock_selftests },
+ { .init = i915_pmu_init,
+ .exit = i915_pmu_exit },
+ { .init = i915_register_pci_driver,
+ .exit = i915_unregister_pci_driver },
+ { .init = i915_perf_sysctl_register,
+ .exit = i915_perf_sysctl_unregister },
};
static int init_progress;
--
2.30.2
[PATCH v2 06/63] cxl/core: Replace unions with struct_group()
Use the newly introduced struct_group_typed() macro to clean up the
declaration of struct cxl_regs.
Cc: Alison Schofield
Cc: Vishal Verma
Cc: Ira Weiny
Cc: Ben Widawsky
Cc: linux-...@vger.kernel.org
Suggested-by: Dan Williams
Link:
https://lore.kernel.org/lkml/1d9a2e6df2a9a35b2cdd50a9a68cac5991e7e5f0.ca...@intel.com
Signed-off-by: Kees Cook
---
drivers/cxl/cxl.h | 61 ++-
1 file changed, 18 insertions(+), 43 deletions(-)
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 53927f9fa77e..9db0c402c9ce 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -75,52 +75,27 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr)
#define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
#define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
-#define CXL_COMPONENT_REGS() \
- void __iomem *hdm_decoder
-
-#define CXL_DEVICE_REGS() \
- void __iomem *status; \
- void __iomem *mbox; \
- void __iomem *memdev
-
-/* See note for 'struct cxl_regs' for the rationale of this organization */
-/*
- * CXL_COMPONENT_REGS - Common set of CXL Component register block base
pointers
- * @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
- */
-struct cxl_component_regs {
- CXL_COMPONENT_REGS();
-};
-
-/* See note for 'struct cxl_regs' for the rationale of this organization */
-/*
- * CXL_DEVICE_REGS - Common set of CXL Device register block base pointers
- * @status: CXL 2.0 8.2.8.3 Device Status Registers
- * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
- * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
- */
-struct cxl_device_regs {
- CXL_DEVICE_REGS();
-};
-
/*
- * Note, the anonymous union organization allows for per
- * register-block-type helper routines, without requiring block-type
- * agnostic code to include the prefix.
+ * Using struct_group() allows for per register-block-type helper routines,
+ * without requiring block-type agnostic code to include the prefix.
*/
struct cxl_regs {
- union {
- struct {
- CXL_COMPONENT_REGS();
- };
- struct cxl_component_regs component;
- };
- union {
- struct {
- CXL_DEVICE_REGS();
- };
- struct cxl_device_regs device_regs;
- };
+ /*
+* Common set of CXL Component register block base pointers
+* @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
+*/
+ struct_group_tagged(cxl_component_regs, component,
+ void __iomem *hdm_decoder;
+ );
+ /*
+* Common set of CXL Device register block base pointers
+* @status: CXL 2.0 8.2.8.3 Device Status Registers
+* @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
+* @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
+*/
+ struct_group_tagged(cxl_device_regs, device_regs,
+ void __iomem *status, *mbox, *memdev;
+ );
};
struct cxl_reg_map {
--
2.30.2
