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

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

Signed-off-by: Kees Cook keescook@chromium.org --- 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;

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 keithpac@amazon.com Signed-off-by: Keith Packard keithpac@amazon.com Signed-off-by: Kees Cook keescook@chromium.org --- include/linux/stddef.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)

diff --git a/include/linux/stddef.h b/include/linux/stddef.h index 998a4ba28eba..cf7f866944f9 100644 --- a/include/linux/stddef.h +++ b/include/linux/stddef.h @@ -36,4 +36,38 @@ enum { #define offsetofend(TYPE, MEMBER) \ (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))

+/** + * struct_group_attr(NAME, ATTRS, MEMBERS) + * + * Used to create an anonymous union of two structs with identical + * layout and size: one anonymous and one named. The former can be + * used normally without sub-struct naming, and the latter can be + * used to reason about the start, end, and size of the group of + * struct members. Includes structure attributes argument. + * + * @NAME: The name of the mirrored sub-struct + * @ATTRS: Any struct attributes (normally empty) + * @MEMBERS: The member declarations for the mirrored structs + */ +#define struct_group_attr(NAME, ATTRS, MEMBERS) \ + union { \ + struct { MEMBERS } ATTRS; \ + struct { MEMBERS } ATTRS NAME; \ + } + +/** + * struct_group(NAME, MEMBERS) + * + * Used to create an anonymous union of two structs with identical + * layout and size: one anonymous and one named. The former can be + * used normally without sub-struct naming, and the latter can be + * used to reason about the start, end, and size of the group of + * struct members. + * + * @NAME: The name of the mirrored sub-struct + * @MEMBERS: The member declarations for the mirrored structs + */ +#define struct_group(NAME, MEMBERS) \ + struct_group_attr(NAME, /* no attrs */, MEMBERS) + #endif

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 keescook@chromium.org --- drivers/net/wireguard/queueing.h | 4 +--- include/linux/skbuff.h | 9 ++++----- net/core/skbuff.c | 14 +++++--------- 3 files changed, 10 insertions(+), 17 deletions(-)

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

- /* fields enclosed in headers_start/headers_end are copied + /* Fields enclosed in headers group are copied * using a single memcpy() in __copy_skb_header() */ - /* private: */ - __u32 headers_start[0]; + struct_group(headers, /* public: */

/* if you move pkt_type around you also must adapt those constants */ @@ -920,8 +919,8 @@ struct sk_buff { u64 kcov_handle; #endif

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

/* These elements must be at the end, see alloc_skb() for details. */ diff --git a/net/core/skbuff.c b/net/core/skbuff.c index fc7942c0dddc..5f29c65507e0 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -987,12 +987,10 @@ void napi_consume_skb(struct sk_buff *skb, int budget) } EXPORT_SYMBOL(napi_consume_skb);

-/* Make sure a field is enclosed inside headers_start/headers_end section */ +/* Make sure a field is contained by headers group */ #define CHECK_SKB_FIELD(field) \ - BUILD_BUG_ON(offsetof(struct sk_buff, field) < \ - offsetof(struct sk_buff, headers_start)); \ - BUILD_BUG_ON(offsetof(struct sk_buff, field) > \ - offsetof(struct sk_buff, headers_end)); \ + BUILD_BUG_ON(offsetof(struct sk_buff, field) != \ + offsetof(struct sk_buff, headers.field)); \

static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) { @@ -1004,14 +1002,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) __skb_ext_copy(new, old); __nf_copy(new, old, false);

- /* Note : this field could be in headers_start/headers_end section + /* Note : this field could be in the headers group. * It is not yet because we do not want to have a 16 bit hole */ new->queue_mapping = old->queue_mapping;

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

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 keescook@chromium.org --- drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c | 4 ++-- drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h | 14 ++++++++------ 2 files changed, 10 insertions(+), 8 deletions(-)

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

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

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

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

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

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 keescook@chromium.org --- .../staging/rtl8192u/ieee80211/ieee80211.h | 24 +++++++++++-------- .../rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 3 ++- .../staging/rtl8192u/ieee80211/ieee80211_rx.c | 8 +++---- 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211.h b/drivers/staging/rtl8192u/ieee80211/ieee80211.h index 7903c777a417..6ae8d7504558 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211.h +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211.h @@ -303,7 +303,7 @@ struct ieee_param { struct { u32 len; u8 reserved[32]; - u8 data[0]; + u8 data[]; } wpa_ie; struct{ int command; @@ -316,7 +316,7 @@ struct ieee_param { u8 idx; u8 seq[8]; /* sequence counter (set: RX, get: TX) */ u16 key_len; - u8 key[0]; + u8 key[]; } crypt; } u; }; @@ -917,9 +917,11 @@ struct rtl_80211_hdr_3addr { struct rtl_80211_hdr_4addr { __le16 frame_ctl; __le16 duration_id; - u8 addr1[ETH_ALEN]; - u8 addr2[ETH_ALEN]; - u8 addr3[ETH_ALEN]; + struct_group(addrs, + u8 addr1[ETH_ALEN]; + u8 addr2[ETH_ALEN]; + u8 addr3[ETH_ALEN]; + ); __le16 seq_ctl; u8 addr4[ETH_ALEN]; u8 payload[]; @@ -1100,11 +1102,13 @@ typedef union _frameqos { struct ieee80211_qos_information_element { u8 elementID; u8 length; - u8 qui[QOS_OUI_LEN]; - u8 qui_type; - u8 qui_subtype; - u8 version; - u8 ac_info; + struct_group(data, + u8 qui[QOS_OUI_LEN]; + u8 qui_type; + u8 qui_subtype; + u8 version; + u8 ac_info; + ); } __packed;

struct ieee80211_qos_ac_parameter { diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c index c241cf484023..61f9a189c90c 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c @@ -141,7 +141,8 @@ static int ccmp_init_iv_and_aad(struct rtl_80211_hdr_4addr *hdr, pos = (u8 *)hdr; aad[0] = pos[0] & 0x8f; aad[1] = pos[1] & 0xc7; - memcpy(aad + 2, hdr->addr1, 3 * ETH_ALEN); + BUILD_BUG_ON(sizeof(hdr->addrs) != 3 * ETH_ALEN); + memcpy(aad + 2, &hdr->addrs, 3 * ETH_ALEN); pos = (u8 *)&hdr->seq_ctl; aad[20] = pos[0] & 0x0f; aad[21] = 0; /* all bits masked */ diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c index b0e01ee65f7f..d5557ee0a3b7 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c @@ -1332,13 +1332,13 @@ static int ieee80211_read_qos_param_element(struct ieee80211_qos_parameter_info *info_element) { int ret = 0; - u16 size = sizeof(struct ieee80211_qos_parameter_info) - 2; + u16 size = sizeof(element_param->info_element.data);

if (!info_element || !element_param) return -1;

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

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

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;

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

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

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

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

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 keescook@chromium.org --- drivers/staging/rtl8723bs/core/rtw_security.c | 5 +++-- drivers/staging/rtl8723bs/core/rtw_xmit.c | 5 +++-- include/linux/ieee80211.h | 8 +++++--- net/wireless/lib80211_crypt_ccmp.c | 3 ++- 4 files changed, 13 insertions(+), 8 deletions(-)

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

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

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 keescook@chromium.org --- drivers/net/wireless/marvell/mwl8k.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)

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

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

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

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 keescook@chromium.org --- drivers/net/wireless/marvell/libertas_tf/libertas_tf.h | 10 ++++++---- drivers/net/wireless/marvell/libertas_tf/main.c | 3 ++- 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h index 5d726545d987..b2af2ddb6bc4 100644 --- a/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h +++ b/drivers/net/wireless/marvell/libertas_tf/libertas_tf.h @@ -268,10 +268,12 @@ struct txpd { __le32 tx_packet_location; /* Tx packet length */ __le16 tx_packet_length; - /* First 2 byte of destination MAC address */ - u8 tx_dest_addr_high[2]; - /* Last 4 byte of destination MAC address */ - u8 tx_dest_addr_low[4]; + struct_group(tx_dest_addr, + /* First 2 byte of destination MAC address */ + u8 tx_dest_addr_high[2]; + /* Last 4 byte of destination MAC address */ + u8 tx_dest_addr_low[4]; + ); /* Pkt Priority */ u8 priority; /* Pkt Trasnit Power control */ diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c b/drivers/net/wireless/marvell/libertas_tf/main.c index 71492211904b..02a1e1f547d8 100644 --- a/drivers/net/wireless/marvell/libertas_tf/main.c +++ b/drivers/net/wireless/marvell/libertas_tf/main.c @@ -232,7 +232,8 @@ static void lbtf_tx_work(struct work_struct *work) ieee80211_get_tx_rate(priv->hw, info)->hw_value);

/* copy destination address from 802.11 header */ - memcpy(txpd->tx_dest_addr_high, skb->data + sizeof(struct txpd) + 4, + BUILD_BUG_ON(sizeof(txpd->tx_dest_addr) != ETH_ALEN); + memcpy(&txpd->tx_dest_addr, skb->data + sizeof(struct txpd) + 4, ETH_ALEN); txpd->tx_packet_length = cpu_to_le16(len); txpd->tx_packet_location = cpu_to_le32(sizeof(struct txpd));

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 keescook@chromium.org --- drivers/iommu/amd/init.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)

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

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

if (!(*hid)) {

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 keescook@chromium.org --- include/net/flow.h | 6 ++++-- include/uapi/linux/if_ether.h | 12 ++++++++++-- include/uapi/linux/ip.h | 12 ++++++++++-- include/uapi/linux/ipv6.h | 12 ++++++++++-- net/core/flow_dissector.c | 10 ++++++---- net/ipv4/ip_output.c | 6 ++---- 6 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/include/net/flow.h b/include/net/flow.h index 6f5e70240071..f1a3b6c8eae2 100644 --- a/include/net/flow.h +++ b/include/net/flow.h @@ -81,8 +81,10 @@ struct flowi4 { #define flowi4_multipath_hash __fl_common.flowic_multipath_hash

/* (saddr,daddr) must be grouped, same order as in IP header */ - __be32 saddr; - __be32 daddr; + struct_group(addrs, + __be32 saddr; + __be32 daddr; + );

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

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

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

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

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

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

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

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

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

/* Note: skb->sk can be different from sk, in case of tunnels */

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 keescook@chromium.org --- drivers/net/ethernet/chelsio/cxgb4/sge.c | 8 +++++--- drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h | 10 ++++++---- drivers/net/ethernet/chelsio/cxgb4vf/sge.c | 7 ++----- 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c index 6a099cb34b12..9080b2c5ffe8 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/sge.c +++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c @@ -1842,8 +1842,10 @@ static netdev_tx_t cxgb4_vf_eth_xmit(struct sk_buff *skb, * (including the VLAN tag) into the header so we reject anything * smaller than that ... */ - fw_hdr_copy_len = sizeof(wr->ethmacdst) + sizeof(wr->ethmacsrc) + - sizeof(wr->ethtype) + sizeof(wr->vlantci); + BUILD_BUG_ON(sizeof(wr->firmware) != + (sizeof(wr->ethmacdst) + sizeof(wr->ethmacsrc) + + sizeof(wr->ethtype) + sizeof(wr->vlantci))); + fw_hdr_copy_len = sizeof(wr->firmware); ret = cxgb4_validate_skb(skb, dev, fw_hdr_copy_len); if (ret) goto out_free; @@ -1924,7 +1926,7 @@ static netdev_tx_t cxgb4_vf_eth_xmit(struct sk_buff *skb, wr->equiq_to_len16 = cpu_to_be32(wr_mid); wr->r3[0] = cpu_to_be32(0); wr->r3[1] = cpu_to_be32(0); - skb_copy_from_linear_data(skb, (void *)wr->ethmacdst, fw_hdr_copy_len); + skb_copy_from_linear_data(skb, &wr->firmware, fw_hdr_copy_len); end = (u64 *)wr + flits;

/* If this is a Large Send Offload packet we'll put in an LSO CPL diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h index 0a326c054707..2419459a0b85 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h +++ b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h @@ -794,10 +794,12 @@ struct fw_eth_tx_pkt_vm_wr { __be32 op_immdlen; __be32 equiq_to_len16; __be32 r3[2]; - u8 ethmacdst[6]; - u8 ethmacsrc[6]; - __be16 ethtype; - __be16 vlantci; + struct_group(firmware, + u8 ethmacdst[ETH_ALEN]; + u8 ethmacsrc[ETH_ALEN]; + __be16 ethtype; + __be16 vlantci; + ); };

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

/* * The chip minimum packet length is 10 octets but the firmware @@ -1267,7 +1264,7 @@ netdev_tx_t t4vf_eth_xmit(struct sk_buff *skb, struct net_device *dev) wr->equiq_to_len16 = cpu_to_be32(wr_mid); wr->r3[0] = cpu_to_be32(0); wr->r3[1] = cpu_to_be32(0); - skb_copy_from_linear_data(skb, (void *)wr->ethmacdst, fw_hdr_copy_len); + skb_copy_from_linear_data(skb, &wr->firmware, fw_hdr_copy_len); end = (u64 *)wr + flits;

/*

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 keescook@chromium.org --- drivers/gpu/drm/amd/include/atomfirmware.h | 9 ++++++++- .../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h | 3 ++- drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++- .../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++- drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 +++--- drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 12 ++++++++---- drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 +++--- 7 files changed, 28 insertions(+), 14 deletions(-)

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

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

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

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

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

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

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

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

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

uint32_t BoardReserved[5]; + ); };

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

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

uint32_t reserved[16]; + ); };

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

// SVI2 Board Parameters + struct_group(v4_6, uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value. uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value.

@@ -728,10 +729,10 @@ typedef struct { uint32_t BoardVoltageCoeffB; // decode by /1000

uint32_t BoardReserved[7]; + );

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

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

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

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

uint32_t BoardReserved[8]; + );

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

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

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

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

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

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

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

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

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

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 keescook@chromium.org --- drivers/staging/wlan-ng/hfa384x.h | 16 +++++++++------- drivers/staging/wlan-ng/hfa384x_usb.c | 4 +++- 2 files changed, 12 insertions(+), 8 deletions(-)

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

/*-- 802.11 Header Information --*/

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

/*-- 802.3 Header Information --*/ diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c index f2a0e16b0318..38aaae7a2d69 100644 --- a/drivers/staging/wlan-ng/hfa384x_usb.c +++ b/drivers/staging/wlan-ng/hfa384x_usb.c @@ -2516,7 +2516,9 @@ int hfa384x_drvr_txframe(struct hfa384x *hw, struct sk_buff *skb, cpu_to_le16s(&hw->txbuff.txfrm.desc.tx_control);

/* copy the header over to the txdesc */ - memcpy(&hw->txbuff.txfrm.desc.frame_control, p80211_hdr, + BUILD_BUG_ON(sizeof(hw->txbuff.txfrm.desc.p80211) != + sizeof(union p80211_hdr)); + memcpy(&hw->txbuff.txfrm.desc.p80211, p80211_hdr, sizeof(union p80211_hdr));

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

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 keescook@chromium.org --- drivers/gpu/drm/mga/mga_ioc32.c | 30 ++++++++++++++------------ include/uapi/drm/mga_drm.h | 37 ++++++++++++++++++++++++--------- 2 files changed, 44 insertions(+), 23 deletions(-)

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

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

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

unsigned long sarea_priv_offset;

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

- unsigned int maccess; + unsigned int maccess;

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

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

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

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

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

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 keescook@chromium.org --- drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +- include/linux/if_vlan.h | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-)

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

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

Since all compilers support __builtin_object_size(), and there is only one user of __compiletime_object_size, remove it to avoid the needless indirection. This lets Clang reason about check_copy_size() correctly.

Link: https://github.com/ClangBuiltLinux/linux/issues/1179 Suggested-by: Nick Desaulniers ndesaulniers@google.com Signed-off-by: Kees Cook keescook@chromium.org --- include/linux/compiler-gcc.h | 2 -- include/linux/compiler_types.h | 4 ---- include/linux/thread_info.h | 2 +- 3 files changed, 1 insertion(+), 7 deletions(-)

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

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

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

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

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

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 keescook@chromium.org --- include/linux/fortify-string.h | 7 +++++++ include/linux/string.h | 9 --------- lib/string_helpers.c | 2 ++ 3 files changed, 9 insertions(+), 9 deletions(-)

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

+#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline)) +#define __RENAME(x) __asm__(#x) + +void fortify_panic(const char *name) __noreturn __cold; +void __read_overflow(void) __compiletime_error("detected read beyond size of object (1st parameter)"); +void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)"); +void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");

#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS) extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size) __RENAME(memchr); diff --git a/include/linux/string.h b/include/linux/string.h index b48d2d28e0b1..9473f81b9db2 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -249,15 +249,6 @@ static inline const char *kbasename(const char *path) return tail ? tail + 1 : path; }

-#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline)) -#define __RENAME(x) __asm__(#x) - -void fortify_panic(const char *name) __noreturn __cold; -void __read_overflow(void) __compiletime_error("detected read beyond size of object passed as 1st parameter"); -void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter"); -void __read_overflow3(void) __compiletime_error("detected read beyond size of object passed as 3rd parameter"); -void __write_overflow(void) __compiletime_error("detected write beyond size of object passed as 1st parameter"); - #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) #include <linux/fortify-string.h> #endif diff --git a/lib/string_helpers.c b/lib/string_helpers.c index bde13612c25d..faa9d8e4e2c5 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -883,9 +883,11 @@ char *strreplace(char *s, char old, char new) } EXPORT_SYMBOL(strreplace);

+#ifdef CONFIG_FORTIFY_SOURCE void fortify_panic(const char *name) { pr_emerg("detected buffer overflow in %s\n", name); BUG(); } EXPORT_SYMBOL(fortify_panic); +#endif /* CONFIG_FORTIFY_SOURCE */

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 keescook@chromium.org --- lib/.gitignore | 2 + lib/Makefile | 30 ++++++++++ lib/test_fortify/read_overflow-memchr.c | 5 ++ lib/test_fortify/read_overflow-memchr_inv.c | 5 ++ lib/test_fortify/read_overflow-memcmp.c | 5 ++ lib/test_fortify/read_overflow-memscan.c | 5 ++ lib/test_fortify/read_overflow2-memcmp.c | 5 ++ lib/test_fortify/read_overflow2-memcpy.c | 5 ++ lib/test_fortify/read_overflow2-memmove.c | 5 ++ lib/test_fortify/test_fortify.h | 31 ++++++++++ lib/test_fortify/write_overflow-memcpy.c | 5 ++ lib/test_fortify/write_overflow-memmove.c | 5 ++ lib/test_fortify/write_overflow-memset.c | 5 ++ lib/test_fortify/write_overflow-strlcpy.c | 5 ++ lib/test_fortify/write_overflow-strncpy.c | 5 ++ lib/test_fortify/write_overflow-strscpy.c | 5 ++ scripts/test_fortify.sh | 64 +++++++++++++++++++++ 17 files changed, 192 insertions(+) create mode 100644 lib/test_fortify/read_overflow-memchr.c create mode 100644 lib/test_fortify/read_overflow-memchr_inv.c create mode 100644 lib/test_fortify/read_overflow-memcmp.c create mode 100644 lib/test_fortify/read_overflow-memscan.c create mode 100644 lib/test_fortify/read_overflow2-memcmp.c create mode 100644 lib/test_fortify/read_overflow2-memcpy.c create mode 100644 lib/test_fortify/read_overflow2-memmove.c create mode 100644 lib/test_fortify/test_fortify.h create mode 100644 lib/test_fortify/write_overflow-memcpy.c create mode 100644 lib/test_fortify/write_overflow-memmove.c create mode 100644 lib/test_fortify/write_overflow-memset.c create mode 100644 lib/test_fortify/write_overflow-strlcpy.c create mode 100644 lib/test_fortify/write_overflow-strncpy.c create mode 100644 lib/test_fortify/write_overflow-strscpy.c create mode 100644 scripts/test_fortify.sh

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

obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o + +# FORTIFY_SOURCE compile-time behavior tests +TEST_FORTIFY_SRCS = $(wildcard $(srctree)/$(src)/test_fortify/*-*.c) +TEST_FORTIFY_LOGS = $(patsubst $(srctree)/$(src)/%.c, %.log, $(TEST_FORTIFY_SRCS)) +TEST_FORTIFY_LOG = test_fortify.log + +quiet_cmd_test_fortify = TEST $@ + cmd_test_fortify = $(CONFIG_SHELL) $(srctree)/scripts/test_fortify.sh \ + $< $@ "$(NM)" $(CC) $(c_flags) \ + $(call cc-disable-warning,fortify-source) + +targets += $(TEST_FORTIFY_LOGS) +clean-files += $(TEST_FORTIFY_LOGS) +clean-files += $(addsuffix .o, $(TEST_FORTIFY_LOGS)) +$(obj)/test_fortify/%.log: $(src)/test_fortify/%.c $(srctree)/scripts/test_fortify.sh \ + $(srctree)/include/linux/fortify-string.h FORCE + $(call if_changed,test_fortify) + +quiet_cmd_gen_fortify_log = GEN $@ + cmd_gen_fortify_log = cat </dev/null $(filter-out FORCE,$^) 2>/dev/null > $@ || true + +targets += $(TEST_FORTIFY_LOG) +clean-files += $(TEST_FORTIFY_LOG) +$(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE + $(call if_changed,gen_fortify_log) + +# Fake dependency to trigger the fortify tests. +ifeq ($(CONFIG_FORTIFY_SOURCE),y) +$(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG) +endif diff --git a/lib/test_fortify/read_overflow-memchr.c b/lib/test_fortify/read_overflow-memchr.c new file mode 100644 index 000000000000..2743084b32af --- /dev/null +++ b/lib/test_fortify/read_overflow-memchr.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memchr(small, 0x7A, sizeof(small) + 1) + +#include "test_fortify.h" diff --git a/lib/test_fortify/read_overflow-memchr_inv.c b/lib/test_fortify/read_overflow-memchr_inv.c new file mode 100644 index 000000000000..b26e1f1bc217 --- /dev/null +++ b/lib/test_fortify/read_overflow-memchr_inv.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memchr_inv(small, 0x7A, sizeof(small) + 1) + +#include "test_fortify.h" diff --git a/lib/test_fortify/read_overflow-memcmp.c b/lib/test_fortify/read_overflow-memcmp.c new file mode 100644 index 000000000000..d5d301ff64ef --- /dev/null +++ b/lib/test_fortify/read_overflow-memcmp.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memcmp(small, large, sizeof(small) + 1) + +#include "test_fortify.h" diff --git a/lib/test_fortify/read_overflow-memscan.c b/lib/test_fortify/read_overflow-memscan.c new file mode 100644 index 000000000000..c1a97f2df0f0 --- /dev/null +++ b/lib/test_fortify/read_overflow-memscan.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memscan(small, 0x7A, sizeof(small) + 1) + +#include "test_fortify.h" diff --git a/lib/test_fortify/read_overflow2-memcmp.c b/lib/test_fortify/read_overflow2-memcmp.c new file mode 100644 index 000000000000..c6091e640f76 --- /dev/null +++ b/lib/test_fortify/read_overflow2-memcmp.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memcmp(large, small, sizeof(small) + 1) + +#include "test_fortify.h" diff --git a/lib/test_fortify/read_overflow2-memcpy.c b/lib/test_fortify/read_overflow2-memcpy.c new file mode 100644 index 000000000000..07b62e56cf16 --- /dev/null +++ b/lib/test_fortify/read_overflow2-memcpy.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memcpy(large, instance.buf, sizeof(large)) + +#include "test_fortify.h" diff --git a/lib/test_fortify/read_overflow2-memmove.c b/lib/test_fortify/read_overflow2-memmove.c new file mode 100644 index 000000000000..34edfab040a3 --- /dev/null +++ b/lib/test_fortify/read_overflow2-memmove.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memmove(large, instance.buf, sizeof(large)) + +#include "test_fortify.h" diff --git a/lib/test_fortify/test_fortify.h b/lib/test_fortify/test_fortify.h new file mode 100644 index 000000000000..125f5c585e4f --- /dev/null +++ b/lib/test_fortify/test_fortify.h @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <linux/kernel.h> +#include <linux/printk.h> +#include <linux/slab.h> +#include <linux/string.h> + +void do_fortify_tests(void); + +# define __BUF_SMALL 16 +# define __BUF_LARGE 32 +struct fortify_object { + int a; + char buf[__BUF_SMALL]; + int c; +}; +const char small_src[__BUF_SMALL] = "AAAAAAAAAAAAAAA"; +const char large_src[__BUF_LARGE] = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + +char small[__BUF_SMALL]; +char large[__BUF_LARGE]; +struct fortify_object instance; + +void do_fortify_tests(void) +{ + /* Normal initializations. */ + memset(&instance, 0x32, sizeof(instance)); + memset(small, 0xA5, sizeof(small)); + memset(large, 0x5A, sizeof(large)); + + TEST; +} diff --git a/lib/test_fortify/write_overflow-memcpy.c b/lib/test_fortify/write_overflow-memcpy.c new file mode 100644 index 000000000000..3b3984e428fb --- /dev/null +++ b/lib/test_fortify/write_overflow-memcpy.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memcpy(instance.buf, large_src, sizeof(large_src)) + +#include "test_fortify.h" diff --git a/lib/test_fortify/write_overflow-memmove.c b/lib/test_fortify/write_overflow-memmove.c new file mode 100644 index 000000000000..640437c3b3e0 --- /dev/null +++ b/lib/test_fortify/write_overflow-memmove.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memmove(instance.buf, large_src, sizeof(large_src)) + +#include "test_fortify.h" diff --git a/lib/test_fortify/write_overflow-memset.c b/lib/test_fortify/write_overflow-memset.c new file mode 100644 index 000000000000..36e34908cfb3 --- /dev/null +++ b/lib/test_fortify/write_overflow-memset.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memset(instance.buf, 0x5A, sizeof(large_src)) + +#include "test_fortify.h" diff --git a/lib/test_fortify/write_overflow-strlcpy.c b/lib/test_fortify/write_overflow-strlcpy.c new file mode 100644 index 000000000000..1883db7c0cd6 --- /dev/null +++ b/lib/test_fortify/write_overflow-strlcpy.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + strlcpy(instance.buf, large_src, sizeof(instance.buf) + 1) + +#include "test_fortify.h" diff --git a/lib/test_fortify/write_overflow-strncpy.c b/lib/test_fortify/write_overflow-strncpy.c new file mode 100644 index 000000000000..b85f079c815d --- /dev/null +++ b/lib/test_fortify/write_overflow-strncpy.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + strncpy(instance.buf, large_src, sizeof(instance.buf) + 1) + +#include "test_fortify.h" diff --git a/lib/test_fortify/write_overflow-strscpy.c b/lib/test_fortify/write_overflow-strscpy.c new file mode 100644 index 000000000000..38feddf377dc --- /dev/null +++ b/lib/test_fortify/write_overflow-strscpy.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + strscpy(instance.buf, large_src, sizeof(instance.buf) + 1) + +#include "test_fortify.h" diff --git a/scripts/test_fortify.sh b/scripts/test_fortify.sh new file mode 100644 index 000000000000..59b362eed2ac --- /dev/null +++ b/scripts/test_fortify.sh @@ -0,0 +1,64 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0-only +set -e + +# Argument 1: Source file to build. +IN="$1" +shift +# Extract just the filename for error messages below. +FILE="${IN##*/}" +# Extract the function name for error messages below. +FUNC="${FILE##*-}" +FUNC="${FUNC%%.*}" +# Extract the symbol to test for in build/symbol test below. +WANT="__${FILE%%-*}" + +# Argument 2: Where to write the build log. +OUT="$1" +shift +TMP="${OUT}.tmp" + +# Argument 3: Path to "nm" tool. +NM="$1" +shift + +# Remaining arguments are: $(CC) $(c_flags) + +# Skip this test, it is currently failing on all compilers. +if [ "$WANT $FUNC" = "__write_overflow strlcpy" ] ; then + echo "skip: unsafe ${FUNC}() usage not checked for '$WANT' in $IN" > "$OUT" + exit 0 +fi + +# Clean up temporary file at exit. +__cleanup() { + rm -f "$TMP" +} +trap __cleanup EXIT + +status= +# Attempt to build a source that is expected to fail with a specific warning. +if "$@" -Werror -c "$IN" -o "$OUT".o 2> "$TMP" ; then + # If the build succeeds, either the test has failed or the the + # warning may only happen at link time (Clang). In that case, + # make sure the expected symbol is unresolved in the symbol list. + # If so, FORTIFY is working for this case. + if ! $NM -A "$OUT".o | grep -m1 "\bU ${WANT}$" >>"$TMP" ; then + status="warning: unsafe ${FUNC}() usage lacked '$WANT' symbol in $IN" + fi +else + # If the build failed, check for the warning in the stderr (gcc). + if ! grep -q -m1 "error:.*\b${WANT}'" "$TMP" ; then + status="warning: unsafe ${FUNC}() usage lacked '$WANT' warning in $IN" + fi +fi + +if [ -n "$status" ]; then + # Report on failure results, including compilation warnings. + echo "$status" | tee "$OUT" >&2 + cat "$TMP" | tee -a "$OUT" >&2 +else + # Report on good results, and save any compilation output to log. + echo "ok: unsafe ${FUNC}() usage correctly detected with '$WANT' in $IN" >"$OUT" + cat "$TMP" >>"$OUT" +fi

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

Signed-off-by: Kees Cook keescook@chromium.org --- arch/x86/boot/compressed/misc.c | 3 ++- arch/x86/lib/memcpy_32.c | 1 + include/linux/fortify-string.h | 21 ++++--------------- .../read_overflow2_field-memmove.c | 5 +++++ .../write_overflow_field-memmove.c | 5 +++++ 5 files changed, 17 insertions(+), 18 deletions(-) create mode 100644 lib/test_fortify/read_overflow2_field-memmove.c create mode 100644 lib/test_fortify/write_overflow_field-memmove.c

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

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

#undef memcpy #undef memset +#undef memmove

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

extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan); __FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size) @@ -385,7 +373,6 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q) /* Don't use these outside the FORITFY_SOURCE implementation */ #undef __underlying_memchr #undef __underlying_memcmp -#undef __underlying_memmove #undef __underlying_memset #undef __underlying_strcat #undef __underlying_strcpy diff --git a/lib/test_fortify/read_overflow2_field-memmove.c b/lib/test_fortify/read_overflow2_field-memmove.c new file mode 100644 index 000000000000..6cc2724c8f62 --- /dev/null +++ b/lib/test_fortify/read_overflow2_field-memmove.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memmove(large, instance.buf, sizeof(instance.buf) + 1) + +#include "test_fortify.h" diff --git a/lib/test_fortify/write_overflow_field-memmove.c b/lib/test_fortify/write_overflow_field-memmove.c new file mode 100644 index 000000000000..377fcf9bb2fd --- /dev/null +++ b/lib/test_fortify/write_overflow_field-memmove.c @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0-only +#define TEST \ + memmove(instance.buf, large, sizeof(instance.buf) + 1) + +#include "test_fortify.h"