| Message ID | 20210727205855.411487-1-keescook@chromium.org |
|---|---|
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
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 <keescook@chromium.org> > --- > 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. -- Gustavo > > 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; > + __u16 config_counter; > + }; > + struct omap3isp_stat_frame frame; > + }; > }; > #endif > > -- > 2.30.2 >
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 <keescook@chromium.org> Orthogonal to your change, it wasn't immediately obvious to me that SRP_MAX_IU_LEN was the correct length to use for an srp_cmd. However, I traversed the nested unions and it does look OK. For good measure I copied Tyrel and Brian. Acked-by: Martin K. Petersen <martin.petersen@oracle.com> > --- > drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c > index e6a3eaaa57d9..7e8beb42d2d3 100644 > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c > @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, > return SCSI_MLQUEUE_HOST_BUSY; > > /* Set up the actual SRP IU */ > + memset(&evt_struct->iu.srp, 0x00, SRP_MAX_IU_LEN); > srp_cmd = &evt_struct->iu.srp.cmd; > - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > srp_cmd->opcode = SRP_CMD; > memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); > int_to_scsilun(lun, &srp_cmd->lun);
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 <keescook@chromium.org> > > --- > > 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
On Tue, Jul 27, 2021 at 01:57:56PM -0700, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), memmove(), and memset(), avoid > intentionally writing across neighboring fields. > > Replace the existing empty member position markers "headers_start" and > "headers_end" with a struct_group(). This will allow memcpy() and sizeof() > to more easily reason about sizes, and improve readability. > > "pahole" shows no size nor member offset changes to struct sk_buff. > "objdump -d" shows no no meaningful object code changes (i.e. only source > line number induced differences and optimizations.) > > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks -- Gustavo > --- > drivers/net/wireguard/queueing.h | 4 +--- > include/linux/skbuff.h | 9 ++++----- > net/core/skbuff.c | 14 +++++--------- > 3 files changed, 10 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h > index 4ef2944a68bc..52da5e963003 100644 > --- a/drivers/net/wireguard/queueing.h > +++ b/drivers/net/wireguard/queueing.h > @@ -79,9 +79,7 @@ static inline void wg_reset_packet(struct sk_buff *skb, bool encapsulating) > u8 sw_hash = skb->sw_hash; > u32 hash = skb->hash; > skb_scrub_packet(skb, true); > - memset(&skb->headers_start, 0, > - offsetof(struct sk_buff, headers_end) - > - offsetof(struct sk_buff, headers_start)); > + memset(&skb->headers, 0, sizeof(skb->headers)); > if (encapsulating) { > skb->l4_hash = l4_hash; > skb->sw_hash = sw_hash; > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index f19190820e63..b4032e9b130e 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -800,11 +800,10 @@ struct sk_buff { > __u8 active_extensions; > #endif > > - /* fields enclosed in headers_start/headers_end are copied > + /* Fields enclosed in headers group are copied > * using a single memcpy() in __copy_skb_header() > */ > - /* private: */ > - __u32 headers_start[0]; > + struct_group(headers, > /* public: */ > > /* if you move pkt_type around you also must adapt those constants */ > @@ -920,8 +919,8 @@ struct sk_buff { > u64 kcov_handle; > #endif > > - /* private: */ > - __u32 headers_end[0]; > + ); /* end headers group */ > + > /* public: */ > > /* These elements must be at the end, see alloc_skb() for details. */ > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index fc7942c0dddc..5f29c65507e0 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -987,12 +987,10 @@ void napi_consume_skb(struct sk_buff *skb, int budget) > } > EXPORT_SYMBOL(napi_consume_skb); > > -/* Make sure a field is enclosed inside headers_start/headers_end section */ > +/* Make sure a field is contained by headers group */ > #define CHECK_SKB_FIELD(field) \ > - BUILD_BUG_ON(offsetof(struct sk_buff, field) < \ > - offsetof(struct sk_buff, headers_start)); \ > - BUILD_BUG_ON(offsetof(struct sk_buff, field) > \ > - offsetof(struct sk_buff, headers_end)); \ > + BUILD_BUG_ON(offsetof(struct sk_buff, field) != \ > + offsetof(struct sk_buff, headers.field)); \ > > static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > { > @@ -1004,14 +1002,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) > __skb_ext_copy(new, old); > __nf_copy(new, old, false); > > - /* Note : this field could be in headers_start/headers_end section > + /* Note : this field could be in the headers group. > * It is not yet because we do not want to have a 16 bit hole > */ > new->queue_mapping = old->queue_mapping; > > - memcpy(&new->headers_start, &old->headers_start, > - offsetof(struct sk_buff, headers_end) - > - offsetof(struct sk_buff, headers_start)); > + memcpy(&new->headers, &old->headers, sizeof(new->headers)); > CHECK_SKB_FIELD(protocol); > CHECK_SKB_FIELD(csum); > CHECK_SKB_FIELD(hash); > -- > 2.30.2 >
On Tue, Jul 27, 2021 at 01:57:59PM -0700, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), memmove(), and memset(), avoid > intentionally writing across neighboring fields. > > Use struct_group() around members addr1, addr2, and addr3 in struct > rtl_80211_hdr_4addr, and members qui, qui_type, qui_subtype, version, > and ac_info in struct ieee80211_qos_information_element, so they can be > referenced together. This will allow memcpy() and sizeof() to more easily > reason about sizes, improve readability, and avoid future warnings about > writing beyond the end of addr1 and qui. Additionally replace zero sized > arrays with flexible arrays in struct ieee_param. > > "pahole" shows no size nor member offset changes to struct > rtl_80211_hdr_4addr nor struct ieee80211_qos_information_element. "objdump > -d" shows no meaningful object code changes (i.e. only source line number > induced differences and optimizations). > > Signed-off-by: Kees Cook <keescook@chromium.org> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Jul 27, 2021 at 01:58:01PM -0700, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), memmove(), and memset(), avoid > intentionally writing across neighboring fields. > > Use struct_group() around members addr1, addr2, and addr3 in struct > ieee80211_hdr so they can be referenced together. This will allow memcpy() > and sizeof() to more easily reason about sizes, improve readability, > and avoid future warnings about writing beyond the end of addr1. > > "pahole" shows no size nor member offset changes to struct ieee80211_hdr. > "objdump -d" shows no meaningful object code changes (i.e. only source > line number induced differences and optimizations). > > Signed-off-by: Kees Cook <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(-) For the staging portion: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), memmove(), and memset(), avoid > intentionally writing across neighboring fields. > > Use struct_group() in struct flowi4, struct ipv4hdr, and struct ipv6hdr > around members saddr and daddr, so they can be referenced together. This > will allow memcpy() and sizeof() to more easily reason about sizes, > improve readability, and avoid future warnings about writing beyond the > end of saddr. > > "pahole" shows no size nor member offset changes to struct flowi4. > "objdump -d" shows no meaningful object code changes (i.e. only source > line number induced differences.) > > Note that since this is a UAPI header, struct_group() has been open > coded. > > Signed-off-by: Kees Cook <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; A union of the same fields in the same structure in the same way? Ah, because struct_group() can not be used here? Still feels odd to see in a userspace-visible header. > + }; > __be16 h_proto; /* packet type ID field */ > } __attribute__((packed)); > #endif > diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h > index e42d13b55cf3..33647a37e56b 100644 > --- a/include/uapi/linux/ip.h > +++ b/include/uapi/linux/ip.h > @@ -100,8 +100,16 @@ struct iphdr { > __u8 ttl; > __u8 protocol; > __sum16 check; > - __be32 saddr; > - __be32 daddr; > + union { > + struct { > + __be32 saddr; > + __be32 daddr; > + } addrs; > + struct { > + __be32 saddr; > + __be32 daddr; > + }; Same here (except you named the first struct addrs, not the second, unlike above). > + }; > /*The options start here. */ > }; > > diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h > index b243a53fa985..1c26d32e733b 100644 > --- a/include/uapi/linux/ipv6.h > +++ b/include/uapi/linux/ipv6.h > @@ -130,8 +130,16 @@ struct ipv6hdr { > __u8 nexthdr; > __u8 hop_limit; > > - struct in6_addr saddr; > - struct in6_addr daddr; > + union { > + struct { > + struct in6_addr saddr; > + struct in6_addr daddr; > + } addrs; > + struct { > + struct in6_addr saddr; > + struct in6_addr daddr; > + }; addrs first? Consistancy is key :) thanks, greg k-h
On 7/28/21 00:55, Greg Kroah-Hartman wrote: > On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote: >> In preparation for FORTIFY_SOURCE performing compile-time and run-time >> field bounds checking for memcpy(), memmove(), and memset(), avoid >> intentionally writing across neighboring fields. >> >> Use struct_group() in struct flowi4, struct ipv4hdr, and struct ipv6hdr >> around members saddr and daddr, so they can be referenced together. This >> will allow memcpy() and sizeof() to more easily reason about sizes, >> improve readability, and avoid future warnings about writing beyond the >> end of saddr. >> >> "pahole" shows no size nor member offset changes to struct flowi4. >> "objdump -d" shows no meaningful object code changes (i.e. only source >> line number induced differences.) >> >> Note that since this is a UAPI header, struct_group() has been open >> coded. >> >> Signed-off-by: Kees Cook <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; > > A union of the same fields in the same structure in the same way? > > Ah, because struct_group() can not be used here? Still feels odd to see > in a userspace-visible header. > >> + }; >> __be16 h_proto; /* packet type ID field */ >> } __attribute__((packed)); >> #endif >> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h >> index e42d13b55cf3..33647a37e56b 100644 >> --- a/include/uapi/linux/ip.h >> +++ b/include/uapi/linux/ip.h >> @@ -100,8 +100,16 @@ struct iphdr { >> __u8 ttl; >> __u8 protocol; >> __sum16 check; >> - __be32 saddr; >> - __be32 daddr; >> + union { >> + struct { >> + __be32 saddr; >> + __be32 daddr; >> + } addrs; >> + struct { >> + __be32 saddr; >> + __be32 daddr; >> + }; > > Same here (except you named the first struct addrs, not the second, > unlike above). > > >> + }; >> /*The options start here. */ >> }; >> >> diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h >> index b243a53fa985..1c26d32e733b 100644 >> --- a/include/uapi/linux/ipv6.h >> +++ b/include/uapi/linux/ipv6.h >> @@ -130,8 +130,16 @@ struct ipv6hdr { >> __u8 nexthdr; >> __u8 hop_limit; >> >> - struct in6_addr saddr; >> - struct in6_addr daddr; >> + union { >> + struct { >> + struct in6_addr saddr; >> + struct in6_addr daddr; >> + } addrs; >> + struct { >> + struct in6_addr saddr; >> + struct in6_addr daddr; >> + }; > > addrs first? Consistancy is key :) I think addrs should be second. In general, I think all newly added non-anonymous structures should be second. Thanks -- Gustavo
On Wed, Jul 28, 2021 at 01:14:33AM -0500, Gustavo A. R. Silva wrote: > > > On 7/28/21 00:55, Greg Kroah-Hartman wrote: > > On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote: > >> In preparation for FORTIFY_SOURCE performing compile-time and run-time > >> field bounds checking for memcpy(), memmove(), and memset(), avoid > >> intentionally writing across neighboring fields. > >> > >> Use struct_group() in struct flowi4, struct ipv4hdr, and struct ipv6hdr > >> around members saddr and daddr, so they can be referenced together. This > >> will allow memcpy() and sizeof() to more easily reason about sizes, > >> improve readability, and avoid future warnings about writing beyond the > >> end of saddr. > >> > >> "pahole" shows no size nor member offset changes to struct flowi4. > >> "objdump -d" shows no meaningful object code changes (i.e. only source > >> line number induced differences.) > >> > >> Note that since this is a UAPI header, struct_group() has been open > >> coded. > >> > >> Signed-off-by: Kees Cook <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; > > > > A union of the same fields in the same structure in the same way? > > > > Ah, because struct_group() can not be used here? Still feels odd to see > > in a userspace-visible header. > > > >> + }; > >> __be16 h_proto; /* packet type ID field */ > >> } __attribute__((packed)); > >> #endif > >> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h > >> index e42d13b55cf3..33647a37e56b 100644 > >> --- a/include/uapi/linux/ip.h > >> +++ b/include/uapi/linux/ip.h > >> @@ -100,8 +100,16 @@ struct iphdr { > >> __u8 ttl; > >> __u8 protocol; > >> __sum16 check; > >> - __be32 saddr; > >> - __be32 daddr; > >> + union { > >> + struct { > >> + __be32 saddr; > >> + __be32 daddr; > >> + } addrs; > >> + struct { > >> + __be32 saddr; > >> + __be32 daddr; > >> + }; > > > > Same here (except you named the first struct addrs, not the second, > > unlike above). > > > > > >> + }; > >> /*The options start here. */ > >> }; > >> > >> diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h > >> index b243a53fa985..1c26d32e733b 100644 > >> --- a/include/uapi/linux/ipv6.h > >> +++ b/include/uapi/linux/ipv6.h > >> @@ -130,8 +130,16 @@ struct ipv6hdr { > >> __u8 nexthdr; > >> __u8 hop_limit; > >> > >> - struct in6_addr saddr; > >> - struct in6_addr daddr; > >> + union { > >> + struct { > >> + struct in6_addr saddr; > >> + struct in6_addr daddr; > >> + } addrs; > >> + struct { > >> + struct in6_addr saddr; > >> + struct in6_addr daddr; > >> + }; > > > > addrs first? Consistancy is key :) > > I think addrs should be second. In general, I think all newly added > non-anonymous structures should be second. Why not use a local version of the macro like was done in the DRM header file, to make it always work the same and more obvious what is happening? If I were a userspace developer and saw the above, I would think that the kernel developers have lost it :) thanks, greg k-h
On 7/28/21 01:19, Greg Kroah-Hartman wrote: > On Wed, Jul 28, 2021 at 01:14:33AM -0500, Gustavo A. R. Silva wrote: >> >> >> On 7/28/21 00:55, Greg Kroah-Hartman wrote: >>> On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote: >>>> In preparation for FORTIFY_SOURCE performing compile-time and run-time >>>> field bounds checking for memcpy(), memmove(), and memset(), avoid >>>> intentionally writing across neighboring fields. >>>> >>>> Use struct_group() in struct flowi4, struct ipv4hdr, and struct ipv6hdr >>>> around members saddr and daddr, so they can be referenced together. This >>>> will allow memcpy() and sizeof() to more easily reason about sizes, >>>> improve readability, and avoid future warnings about writing beyond the >>>> end of saddr. >>>> >>>> "pahole" shows no size nor member offset changes to struct flowi4. >>>> "objdump -d" shows no meaningful object code changes (i.e. only source >>>> line number induced differences.) >>>> >>>> Note that since this is a UAPI header, struct_group() has been open >>>> coded. >>>> >>>> Signed-off-by: Kees Cook <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; >>> >>> A union of the same fields in the same structure in the same way? >>> >>> Ah, because struct_group() can not be used here? Still feels odd to see >>> in a userspace-visible header. >>> >>>> + }; >>>> __be16 h_proto; /* packet type ID field */ >>>> } __attribute__((packed)); >>>> #endif >>>> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h >>>> index e42d13b55cf3..33647a37e56b 100644 >>>> --- a/include/uapi/linux/ip.h >>>> +++ b/include/uapi/linux/ip.h >>>> @@ -100,8 +100,16 @@ struct iphdr { >>>> __u8 ttl; >>>> __u8 protocol; >>>> __sum16 check; >>>> - __be32 saddr; >>>> - __be32 daddr; >>>> + union { >>>> + struct { >>>> + __be32 saddr; >>>> + __be32 daddr; >>>> + } addrs; >>>> + struct { >>>> + __be32 saddr; >>>> + __be32 daddr; >>>> + }; >>> >>> Same here (except you named the first struct addrs, not the second, >>> unlike above). >>> >>> >>>> + }; >>>> /*The options start here. */ >>>> }; >>>> >>>> diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h >>>> index b243a53fa985..1c26d32e733b 100644 >>>> --- a/include/uapi/linux/ipv6.h >>>> +++ b/include/uapi/linux/ipv6.h >>>> @@ -130,8 +130,16 @@ struct ipv6hdr { >>>> __u8 nexthdr; >>>> __u8 hop_limit; >>>> >>>> - struct in6_addr saddr; >>>> - struct in6_addr daddr; >>>> + union { >>>> + struct { >>>> + struct in6_addr saddr; >>>> + struct in6_addr daddr; >>>> + } addrs; >>>> + struct { >>>> + struct in6_addr saddr; >>>> + struct in6_addr daddr; >>>> + }; >>> >>> addrs first? Consistancy is key :) >> >> I think addrs should be second. In general, I think all newly added >> non-anonymous structures should be second. > > Why not use a local version of the macro like was done in the DRM header > file, to make it always work the same and more obvious what is > happening? If I were a userspace developer and saw the above, I would > think that the kernel developers have lost it :) Then don't take a look at this[1]. :p -- Gustavo [1] https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d
On 7/28/21 01:31, Gustavo A. R. Silva wrote: > > > On 7/28/21 01:19, Greg Kroah-Hartman wrote: >> On Wed, Jul 28, 2021 at 01:14:33AM -0500, Gustavo A. R. Silva wrote: >>> >>> >>> On 7/28/21 00:55, Greg Kroah-Hartman wrote: >>>> On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote: >>>>> In preparation for FORTIFY_SOURCE performing compile-time and run-time >>>>> field bounds checking for memcpy(), memmove(), and memset(), avoid >>>>> intentionally writing across neighboring fields. >>>>> >>>>> Use struct_group() in struct flowi4, struct ipv4hdr, and struct ipv6hdr >>>>> around members saddr and daddr, so they can be referenced together. This >>>>> will allow memcpy() and sizeof() to more easily reason about sizes, >>>>> improve readability, and avoid future warnings about writing beyond the >>>>> end of saddr. >>>>> >>>>> "pahole" shows no size nor member offset changes to struct flowi4. >>>>> "objdump -d" shows no meaningful object code changes (i.e. only source >>>>> line number induced differences.) >>>>> >>>>> Note that since this is a UAPI header, struct_group() has been open >>>>> coded. >>>>> >>>>> Signed-off-by: Kees Cook <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; >>>> >>>> A union of the same fields in the same structure in the same way? >>>> >>>> Ah, because struct_group() can not be used here? Still feels odd to see >>>> in a userspace-visible header. >>>> >>>>> + }; >>>>> __be16 h_proto; /* packet type ID field */ >>>>> } __attribute__((packed)); >>>>> #endif >>>>> diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h >>>>> index e42d13b55cf3..33647a37e56b 100644 >>>>> --- a/include/uapi/linux/ip.h >>>>> +++ b/include/uapi/linux/ip.h >>>>> @@ -100,8 +100,16 @@ struct iphdr { >>>>> __u8 ttl; >>>>> __u8 protocol; >>>>> __sum16 check; >>>>> - __be32 saddr; >>>>> - __be32 daddr; >>>>> + union { >>>>> + struct { >>>>> + __be32 saddr; >>>>> + __be32 daddr; >>>>> + } addrs; >>>>> + struct { >>>>> + __be32 saddr; >>>>> + __be32 daddr; >>>>> + }; >>>> >>>> Same here (except you named the first struct addrs, not the second, >>>> unlike above). >>>> >>>> >>>>> + }; >>>>> /*The options start here. */ >>>>> }; >>>>> >>>>> diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h >>>>> index b243a53fa985..1c26d32e733b 100644 >>>>> --- a/include/uapi/linux/ipv6.h >>>>> +++ b/include/uapi/linux/ipv6.h >>>>> @@ -130,8 +130,16 @@ struct ipv6hdr { >>>>> __u8 nexthdr; >>>>> __u8 hop_limit; >>>>> >>>>> - struct in6_addr saddr; >>>>> - struct in6_addr daddr; >>>>> + union { >>>>> + struct { >>>>> + struct in6_addr saddr; >>>>> + struct in6_addr daddr; >>>>> + } addrs; >>>>> + struct { >>>>> + struct in6_addr saddr; >>>>> + struct in6_addr daddr; >>>>> + }; >>>> >>>> addrs first? Consistancy is key :) >>> >>> I think addrs should be second. In general, I think all newly added >>> non-anonymous structures should be second. >> >> Why not use a local version of the macro like was done in the DRM header >> file, to make it always work the same and more obvious what is Yep; I agree. That one looks just fine. :) -- Gustavo
On Wed, Jul 28, 2021 at 01:31:16AM -0500, Gustavo A. R. Silva wrote: > > Why not use a local version of the macro like was done in the DRM header > > file, to make it always work the same and more obvious what is > > happening? If I were a userspace developer and saw the above, I would > > think that the kernel developers have lost it :) > > Then don't take a look at this[1]. :p > > -- > Gustavo > > [1] https://git.kernel.org/linus/c0a744dcaa29e9537e8607ae9c965ad936124a4d That one at least looks a "little" different so maybe it could be seen as semi-reasonable :)
On Tue, Jul 27, 2021 at 01: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 <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; 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; /* 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; /* 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_TSFT); + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT); pos += 8; } @@ -396,7 +396,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, *pos = 0; } else { int shift = 0; - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE); + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE); if (status->bw == RATE_INFO_BW_10) shift = 1; else if (status->bw == RATE_INFO_BW_5) @@ -432,7 +432,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, if (ieee80211_hw_check(&local->hw, SIGNAL_DBM) && !(status->flag & RX_FLAG_NO_SIGNAL_VAL)) { *pos = status->signal; - rthdr->it_present |= + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_DBM_ANTSIGNAL); pos++; } @@ -459,7 +459,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, if (status->encoding == RX_ENC_HT) { unsigned int stbc; - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_MCS); + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_MCS); *pos++ = local->hw.radiotap_mcs_details; *pos = 0; if (status->enc_flags & RX_ENC_FLAG_SHORT_GI) @@ -482,7 +482,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, /* ensure 4 byte alignment */ while ((pos - (u8 *)rthdr) & 3) pos++; - rthdr->it_present |= + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_AMPDU_STATUS); put_unaligned_le32(status->ampdu_reference, pos); pos += 4; @@ -510,7 +510,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, if (status->encoding == RX_ENC_VHT) { u16 known = local->hw.radiotap_vht_details; - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_VHT); + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_VHT); put_unaligned_le16(known, pos); pos += 2; /* flags */ @@ -553,7 +553,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, u16 accuracy = 0; u8 flags = IEEE80211_RADIOTAP_TIMESTAMP_FLAG_32BIT; - rthdr->it_present |= + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TIMESTAMP); /* ensure 8 byte alignment */ @@ -642,7 +642,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, /* ensure 2 byte alignment */ while ((pos - (u8 *)rthdr) & 1) pos++; - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE); + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE); memcpy(pos, &he, sizeof(he)); pos += sizeof(he); } @@ -652,13 +652,13 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, /* ensure 2 byte alignment */ while ((pos - (u8 *)rthdr) & 1) pos++; - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE_MU); + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE_MU); memcpy(pos, &he_mu, sizeof(he_mu)); pos += sizeof(he_mu); } if (status->flag & RX_FLAG_NO_PSDU) { - rthdr->it_present |= + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_ZERO_LEN_PSDU); *pos++ = status->zero_length_psdu_type; } @@ -667,7 +667,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, /* ensure 2 byte alignment */ while ((pos - (u8 *)rthdr) & 1) pos++; - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_LSIG); + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_LSIG); memcpy(pos, &lsig, sizeof(lsig)); pos += sizeof(lsig); } diff --git a/net/wireless/radiotap.c b/net/wireless/radiotap.c index 36f1b59a78bf..f7852024c011 100644 --- a/net/wireless/radiotap.c +++ b/net/wireless/radiotap.c @@ -114,11 +114,10 @@ int ieee80211_radiotap_iterator_init( iterator->_rtheader = radiotap_header; 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->_bitmap_shifter = get_unaligned_le32(&radiotap_header->data.it_present); iterator->_arg = (uint8_t *)radiotap_header + sizeof(*radiotap_header); iterator->_reset_on_ext = 0; - iterator->_next_bitmap = &radiotap_header->it_present; - iterator->_next_bitmap++; + iterator->_next_bitmap = (__le32 *)&radiotap_header->data.buff; iterator->_vns = vns; iterator->current_namespace = &radiotap_ns; iterator->is_radiotap_ns = 1;
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 <keescook@chromium.org> > --- > 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 = { }; Should this be { 0 } ? We've seen patches trying to switch from { 0 } to { } but the answer was that { 0 } is supposed to be used, http://www.ex-parrot.com/~chris/random/initialise.html (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/)
On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote: > > 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 = { }; > > Should this be { 0 } ? > > We've seen patches trying to switch from { 0 } to { } but the answer > was that { 0 } is supposed to be used, > http://www.ex-parrot.com/~chris/random/initialise.html > > (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/) In the kernel we don't care about portability so much. Use the = { } GCC extension. If the first member of the struct is a pointer then Sparse will complain about = { 0 }. I had a patch to make checkpatch.pl complain about = { 0 }; but my system died and I haven't transfered my postponed messages to the new system... regards, dan carpenter
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 0xfffffffff80000000. 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.
On 27/07/2021 22.58, Kees Cook wrote: > At its core, FORTIFY_SOURCE uses the compiler's __builtin_object_size() > internal[0] to determine the available size at a target address based on > the compile-time known structure layout details. It operates in two > modes: outer bounds (0) and inner bounds (1). In mode 0, the size of the > enclosing structure is used. In mode 1, the size of the specific field > is used. For example: > > struct object { > u16 scalar1; /* 2 bytes */ > char array[6]; /* 6 bytes */ > u64 scalar2; /* 8 bytes */ > u32 scalar3; /* 4 bytes */ > } instance; > > > __builtin_object_size(instance.array, 0) == 18, since the remaining size > of the enclosing structure starting from "array" is 18 bytes (6 + 8 + 4). I think the compiler would usually end up making that struct size 24, with 4 bytes of trailing padding (at least when alignof(u64) is 8). In that case, does __builtin_object_size(instance.array, 0) actually evaluate to 18, or to 22? A quick test on x86-64 suggests the latter, so the memcpy(, , 20) would not be a violation. Perhaps it's better to base the example on something which doesn't have potential trailing padding - so either add another 4 byte member, or also make scalar2 u32. Rasmus
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 <keescook@chromium.org> > > Orthogonal to your change, it wasn't immediately obvious to me that > SRP_MAX_IU_LEN was the correct length to use for an srp_cmd. However, I > traversed the nested unions and it does look OK. Yeah, I had the same fun. Maybe I should add a BUILD_BUG_ON() here to help illustrate the relationship? I did that in a few other places where the equalities weren't very clear. For example, change it to: + BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN); + memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp)); srp_cmd = &evt_struct->iu.srp.cmd; - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > > For good measure I copied Tyrel and Brian. > > Acked-by: Martin K. Petersen <martin.petersen@oracle.com> For the moment, I'll leave the patch as-is unless you prefer having the BUILD_BUG_ON(). :) Thanks! -Kees > > > --- > > drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c > > index e6a3eaaa57d9..7e8beb42d2d3 100644 > > --- a/drivers/scsi/ibmvscsi/ibmvscsi.c > > +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c > > @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, > > return SCSI_MLQUEUE_HOST_BUSY; > > > > /* Set up the actual SRP IU */ > > + memset(&evt_struct->iu.srp, 0x00, SRP_MAX_IU_LEN); > > srp_cmd = &evt_struct->iu.srp.cmd; > > - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > > srp_cmd->opcode = SRP_CMD; > > memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); > > int_to_scsilun(lun, &srp_cmd->lun); > > -- > Martin K. Petersen Oracle Linux Engineering
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...)
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 <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; > > 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_TSFT); > + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT); > pos += 8; > } > > @@ -396,7 +396,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, > *pos = 0; > } else { > int shift = 0; > - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE); > + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE); > if (status->bw == RATE_INFO_BW_10) > shift = 1; > else if (status->bw == RATE_INFO_BW_5) > @@ -432,7 +432,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, > if (ieee80211_hw_check(&local->hw, SIGNAL_DBM) && > !(status->flag & RX_FLAG_NO_SIGNAL_VAL)) { > *pos = status->signal; > - rthdr->it_present |= > + rthdr->data.it_present |= > cpu_to_le32(1 << IEEE80211_RADIOTAP_DBM_ANTSIGNAL); > pos++; > } > @@ -459,7 +459,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, > if (status->encoding == RX_ENC_HT) { > unsigned int stbc; > > - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_MCS); > + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_MCS); > *pos++ = local->hw.radiotap_mcs_details; > *pos = 0; > if (status->enc_flags & RX_ENC_FLAG_SHORT_GI) > @@ -482,7 +482,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, > /* ensure 4 byte alignment */ > while ((pos - (u8 *)rthdr) & 3) > pos++; > - rthdr->it_present |= > + rthdr->data.it_present |= > cpu_to_le32(1 << IEEE80211_RADIOTAP_AMPDU_STATUS); > put_unaligned_le32(status->ampdu_reference, pos); > pos += 4; > @@ -510,7 +510,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, > if (status->encoding == RX_ENC_VHT) { > u16 known = local->hw.radiotap_vht_details; > > - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_VHT); > + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_VHT); > put_unaligned_le16(known, pos); > pos += 2; > /* flags */ > @@ -553,7 +553,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, > u16 accuracy = 0; > u8 flags = IEEE80211_RADIOTAP_TIMESTAMP_FLAG_32BIT; > > - rthdr->it_present |= > + rthdr->data.it_present |= > cpu_to_le32(1 << IEEE80211_RADIOTAP_TIMESTAMP); > > /* ensure 8 byte alignment */ > @@ -642,7 +642,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, > /* ensure 2 byte alignment */ > while ((pos - (u8 *)rthdr) & 1) > pos++; > - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE); > + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE); > memcpy(pos, &he, sizeof(he)); > pos += sizeof(he); > } > @@ -652,13 +652,13 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, > /* ensure 2 byte alignment */ > while ((pos - (u8 *)rthdr) & 1) > pos++; > - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE_MU); > + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE_MU); > memcpy(pos, &he_mu, sizeof(he_mu)); > pos += sizeof(he_mu); > } > > if (status->flag & RX_FLAG_NO_PSDU) { > - rthdr->it_present |= > + rthdr->data.it_present |= > cpu_to_le32(1 << IEEE80211_RADIOTAP_ZERO_LEN_PSDU); > *pos++ = status->zero_length_psdu_type; > } > @@ -667,7 +667,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, > /* ensure 2 byte alignment */ > while ((pos - (u8 *)rthdr) & 1) > pos++; > - rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_LSIG); > + rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_LSIG); > memcpy(pos, &lsig, sizeof(lsig)); > pos += sizeof(lsig); > } > diff --git a/net/wireless/radiotap.c b/net/wireless/radiotap.c > index 36f1b59a78bf..f7852024c011 100644 > --- a/net/wireless/radiotap.c > +++ b/net/wireless/radiotap.c > @@ -114,11 +114,10 @@ int ieee80211_radiotap_iterator_init( > iterator->_rtheader = radiotap_header; > 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->_bitmap_shifter = get_unaligned_le32(&radiotap_header->data.it_present); > iterator->_arg = (uint8_t *)radiotap_header + sizeof(*radiotap_header); > iterator->_reset_on_ext = 0; > - iterator->_next_bitmap = &radiotap_header->it_present; > - iterator->_next_bitmap++; > + iterator->_next_bitmap = (__le32 *)&radiotap_header->data.buff; > iterator->_vns = vns; > iterator->current_namespace = &radiotap_ns; > iterator->is_radiotap_ns = 1;
On 7/28/21 2:14 AM, Dan Carpenter wrote: > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote: >>> 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 = { }; >> >> Should this be { 0 } ? >> >> We've seen patches trying to switch from { 0 } to { } but the answer >> was that { 0 } is supposed to be used, >> http://www.ex-parrot.com/~chris/random/initialise.html >> >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/) > > In the kernel we don't care about portability so much. Use the = { } > GCC extension. If the first member of the struct is a pointer then > Sparse will complain about = { 0 }. +1 for { }. BTW, my understanding is that neither the C standard nor the C++ standard guarantee anything about initialization of padding bytes nor about the initialization of unnamed bitfields for stack variables when using aggregate initialization. Bart.
On Wed, Jul 28, 2021 at 02:37:20PM -0700, Bart Van Assche wrote: > On 7/28/21 2:14 AM, Dan Carpenter wrote: > > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote: > >>> 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 = { }; > >> > >> Should this be { 0 } ? > >> > >> We've seen patches trying to switch from { 0 } to { } but the answer > >> was that { 0 } is supposed to be used, > >> http://www.ex-parrot.com/~chris/random/initialise.html > >> > >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/) > > > > In the kernel we don't care about portability so much. Use the = { } > > GCC extension. If the first member of the struct is a pointer then > > Sparse will complain about = { 0 }. > > +1 for { }. Oh, I thought the tendency is is to use { 0 } because that can also intialize the compound members, by a "scalar 0" as it appears in the code.
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 0xfffffffff80000000. > > 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.
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 <keithpac@amazon.com> > > Signed-off-by: Keith Packard <keithpac@amazon.com> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > 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
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 <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; > > 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
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;
On 7/28/21 2:01 PM, Kees Cook wrote: > On Wed, Jul 28, 2021 at 07:55:53AM +0200, Greg Kroah-Hartman wrote: >>> struct ethhdr { >>> - unsigned char h_dest[ETH_ALEN]; /* destination eth addr */ >>> - unsigned char h_source[ETH_ALEN]; /* source ether addr */ >>> + union { >>> + struct { >>> + unsigned char h_dest[ETH_ALEN]; /* destination eth addr */ >>> + unsigned char h_source[ETH_ALEN]; /* source ether addr */ >>> + }; >>> + struct { >>> + unsigned char h_dest[ETH_ALEN]; /* destination eth addr */ >>> + unsigned char h_source[ETH_ALEN]; /* source ether addr */ >>> + } addrs; >> >> A union of the same fields in the same structure in the same way? >> >> Ah, because struct_group() can not be used here? Still feels odd to see >> in a userspace-visible header. > > Yeah, there is some inconsistency here. I will clean this up for v2. > > Is there a place we can put kernel-specific macros for use in UAPI > headers? (I need to figure out where things like __kernel_size_t get > defined...) How about using two memset() calls to clear h_dest[] and h_source[] instead of modifying the uapi header? Thanks, Bart.
Kees, > For example, change it to: > > + BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN); > + memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp)); > srp_cmd = &evt_struct->iu.srp.cmd; > - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > For the moment, I'll leave the patch as-is unless you prefer having > the BUILD_BUG_ON(). :) I'm OK with the BUILD_BUG_ON(). Hopefully Tyrel or Brian will chime in.
On Wed, Jul 28, 2021 at 11:37:30PM +0200, David Sterba wrote: > On Wed, Jul 28, 2021 at 02:37:20PM -0700, Bart Van Assche wrote: > > On 7/28/21 2:14 AM, Dan Carpenter wrote: > > > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote: > > >>> 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 = { }; > > >> > > >> Should this be { 0 } ? > > >> > > >> We've seen patches trying to switch from { 0 } to { } but the answer > > >> was that { 0 } is supposed to be used, > > >> http://www.ex-parrot.com/~chris/random/initialise.html > > >> > > >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/) > > > > > > In the kernel we don't care about portability so much. Use the = { } > > > GCC extension. If the first member of the struct is a pointer then > > > Sparse will complain about = { 0 }. > > > > +1 for { }. > > Oh, I thought the tendency is is to use { 0 } because that can also > intialize the compound members, by a "scalar 0" as it appears in the > code. > Holes in the structure might not be initialized to anything if you do either one of these as well. Or did we finally prove that is not the case? I can not remember anymore... greg k-h
On Thu, Jul 29, 2021 at 07:56:27AM +0200, Greg Kroah-Hartman wrote: > On Wed, Jul 28, 2021 at 11:37:30PM +0200, David Sterba wrote: > > On Wed, Jul 28, 2021 at 02:37:20PM -0700, Bart Van Assche wrote: > > > On 7/28/21 2:14 AM, Dan Carpenter wrote: > > > > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote: > > > >>> 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 = { }; > > > >> > > > >> Should this be { 0 } ? > > > >> > > > >> We've seen patches trying to switch from { 0 } to { } but the answer > > > >> was that { 0 } is supposed to be used, > > > >> http://www.ex-parrot.com/~chris/random/initialise.html > > > >> > > > >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/ ) > > > > > > > > In the kernel we don't care about portability so much. Use the = { } > > > > GCC extension. If the first member of the struct is a pointer then > > > > Sparse will complain about = { 0 }. > > > > > > +1 for { }. > > > > Oh, I thought the tendency is is to use { 0 } because that can also > > intialize the compound members, by a "scalar 0" as it appears in the > > code. > > > > Holes in the structure might not be initialized to anything if you do > either one of these as well. > > Or did we finally prove that is not the case? I can not remember > anymore... Yep. The C11 spec says that struct holes are initialized. https://lore.kernel.org/netdev/20200731140452.GE24045@ziepe.ca/ What doesn't initialize struct holes is assignments: struct foo foo = *bar; regards, dan carpenter
On Wed, Jul 28, 2021 at 04:33:18PM -0700, Kees Cook wrote: > > Ah-ha, got it: > Thanks, Kees! Nice! regards, dan carpenter
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 0xfffffffff80000000. > > > > 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. Looking for all the definitions, there are a few that are wrong in the sense they're using the singed type, eg. https://elixir.bootlin.com/linux/v5.14-rc3/source/arch/arm/mach-davinci/sleep.S#L7 #define BIT(nr) (1 << (nr)) ... #define DEEPSLEEP_SLEEPENABLE_BIT BIT(31) but that's an assembly file so the C integer promotions don't apply. https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/staging/rtl8723bs/include/osdep_service.h#L18 https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/staging/rtl8723bs/include/wifi.h#L15 https://elixir.bootlin.com/linux/v5.14-rc3/source/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c#L15 #define BIT(x) (1 << (x)) Auditing and cleaning that up is for another series, yeah, I'm just pointing it here if somebody feels like doing the work. It's IMO low hanging fruit but can reveal real bugs.
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.
On Thu, Jul 29, 2021 at 11:20:39AM +0300, Dan Carpenter wrote: > On Thu, Jul 29, 2021 at 07:56:27AM +0200, Greg Kroah-Hartman wrote: > > On Wed, Jul 28, 2021 at 11:37:30PM +0200, David Sterba wrote: > > > On Wed, Jul 28, 2021 at 02:37:20PM -0700, Bart Van Assche wrote: > > > > On 7/28/21 2:14 AM, Dan Carpenter wrote: > > > > > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote: > > > > >>> 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 = { }; > > > > >> > > > > >> Should this be { 0 } ? > > > > >> > > > > >> We've seen patches trying to switch from { 0 } to { } but the answer > > > > >> was that { 0 } is supposed to be used, > > > > >> http://www.ex-parrot.com/~chris/random/initialise.html > > > > >> > > > > >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/ ) > > > > > > > > > > In the kernel we don't care about portability so much. Use the = { } > > > > > GCC extension. If the first member of the struct is a pointer then > > > > > Sparse will complain about = { 0 }. > > > > > > > > +1 for { }. > > > > > > Oh, I thought the tendency is is to use { 0 } because that can also > > > intialize the compound members, by a "scalar 0" as it appears in the > > > code. > > > > > > > Holes in the structure might not be initialized to anything if you do > > either one of these as well. > > > > Or did we finally prove that is not the case? I can not remember > > anymore... > > Yep. The C11 spec says that struct holes are initialized. > > https://lore.kernel.org/netdev/20200731140452.GE24045@ziepe.ca/ This is, unfortunately, misleading. The frustrating key word is "partial" in "updated in C11 to require zero'ing padding when doing partial initialization of aggregates". If one initializes _all_ the struct members ... the padding doesn't get initialized. :( (And until recently, _trailing_ padding wasn't getting initialized even when other paddings were.) I've tried to collect all the different ways the compiler might initialize a variable in this test: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/tree/lib/test_stackinit.c?h=for-next/kspp FWIW, there's no difference between -std=gnu99 and -std=c11, and the test shows that padding is _not_ universally initialized (unless your compiler supports -ftrivial-auto-var-init=zero, which Clang does, and GCC will shortly[1]). Running this with GCC 10.3.0, I see this... As expected, having no initializer leaves padding (as well as members) uninitialized: stackinit: small_hole_none FAIL (uninit bytes: 24) stackinit: big_hole_none FAIL (uninit bytes: 128) stackinit: trailing_hole_none FAIL (uninit bytes: 32) Here, "zero" means "= { };" and they get padding initialized: stackinit: small_hole_zero ok stackinit: big_hole_zero ok stackinit: trailing_hole_zero ok Here, "static_partial" means "= { .one_member = 0 };", and "dynamic_partial" means "= { .one_member = some_variable };". These are similarly initialized: stackinit: small_hole_static_partial ok stackinit: big_hole_static_partial ok stackinit: trailing_hole_static_partial ok stackinit: small_hole_dynamic_partial ok stackinit: big_hole_dynamic_partial ok stackinit: trailing_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
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 0xfffffffff80000000. > > > > > > 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. :)
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. regards, dan carpenter
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.)
On 7/27/21 6:39 PM, 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 <keescook@chromium.org> > > Orthogonal to your change, it wasn't immediately obvious to me that > SRP_MAX_IU_LEN was the correct length to use for an srp_cmd. However, I > traversed the nested unions and it does look OK. > > For good measure I copied Tyrel and Brian. LGTM Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com> > > Acked-by: Martin K. Petersen <martin.petersen@oracle.com> > >> --- >> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >> index e6a3eaaa57d9..7e8beb42d2d3 100644 >> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c >> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c >> @@ -1055,8 +1055,8 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, >> return SCSI_MLQUEUE_HOST_BUSY; >> >> /* Set up the actual SRP IU */ >> + memset(&evt_struct->iu.srp, 0x00, SRP_MAX_IU_LEN); >> srp_cmd = &evt_struct->iu.srp.cmd; >> - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); >> srp_cmd->opcode = SRP_CMD; >> memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); >> int_to_scsilun(lun, &srp_cmd->lun); >
On 7/28/21 8:35 PM, Martin K. Petersen wrote: > > Kees, > >> For example, change it to: >> >> + BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN); >> + memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp)); >> srp_cmd = &evt_struct->iu.srp.cmd; >> - memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); > >> For the moment, I'll leave the patch as-is unless you prefer having >> the BUILD_BUG_ON(). :) > > I'm OK with the BUILD_BUG_ON(). Hopefully Tyrel or Brian will chime in. > All the other srp structs are at most 64 bytes and the size of the union is explicitly set to SRP_MAX_IU_LEN by the last field of the union. union srp_iu { struct srp_login_req login_req; struct srp_login_rsp login_rsp; struct srp_login_rej login_rej; struct srp_i_logout i_logout; struct srp_t_logout t_logout; struct srp_tsk_mgmt tsk_mgmt; struct srp_cmd cmd; struct srp_rsp rsp; u8 reserved[SRP_MAX_IU_LEN]; }; So, in my mind if SRP_MAX_IU_LEN ever changes so does the size of the union making the BUILD_BUG_ON() superfluous. But it doesn't really hurt anything either. -Tyrel
On Fri, Jul 30, 2021 at 10:19:20PM +0000, 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.h > @@ -49,12 +49,18 @@ enum { > * @ATTRS: Any struct attributes (normally empty) > * @MEMBERS: The member declarations for the mirrored structs > */ > -#define struct_group_attr(NAME, ATTRS, MEMBERS) \ > +#define struct_group_attr(NAME, ATTRS, MEMBERS...) \ > union { \ > struct { MEMBERS } ATTRS; \ > struct { MEMBERS } ATTRS NAME; \ > } > > +#define struct_group_attr_typed(TYPE, NAME, ATTRS, MEMBERS...) \ > + union { \ > + struct { MEMBERS } ATTRS; \ > + struct TYPE { MEMBERS } ATTRS NAME; \ > + } > + > /** > * 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...)
On Sat, Jul 31, 2021 at 07:24:44AM +0200, Rasmus Villemoes wrote: > On Sat, Jul 31, 2021, 04:59 Kees Cook <keescook@chromium.org> wrote: > > > On Fri, Jul 30, 2021 at 10:19:20PM +0000, 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;
Hi, This patch series (based on next-20210726) implements stricter (no struct member overflows) bounds checking for memcpy(), memmove(), and memset() under CONFIG_FORTIFY_SOURCE. To quote a later patch in the series: tl;dr: In order to eliminate a large class of common buffer overflow flaws that continue to persist in the kernel, have memcpy() (under CONFIG_FORTIFY_SOURCE) perform bounds checking of the destination struct member when they have a known size. This would have caught all of the memcpy()-related buffer write overflow flaws identified in at least the last three years. As this series introduces various helpers and performs several phases of treewide cleanups, I'm expecting to carry this series in my tree, so I'd love to get some Reviews and Acks. Given the size, I've mostly aimed this series at various mailing lists, otherwise the CC size got really big. :) Specifically, this series is logically split into several steps: Clean up remaining simple compile-time memcpy() warnings: media: omap3isp: Extract struct group for memcpy() region mac80211: Use flex-array for radiotap header bitmap rpmsg: glink: Replace strncpy() with strscpy_pad() Introduce struct_group() and apply it treewide to avoid compile-time memcpy() warnings: stddef: Introduce struct_group() helper macro skbuff: Switch structure bounds to struct_group() bnxt_en: Use struct_group_attr() for memcpy() region staging: rtl8192e: Use struct_group() for memcpy() region staging: rtl8192u: Use struct_group() for memcpy() region staging: rtl8723bs: Avoid field-overflowing memcpy() lib80211: Use struct_group() for memcpy() region net/mlx5e: Avoid field-overflowing memcpy() mwl8k: Use struct_group() for memcpy() region libertas: Use struct_group() for memcpy() region libertas_tf: Use struct_group() for memcpy() region ipw2x00: Use struct_group() for memcpy() region thermal: intel: int340x_thermal: Use struct_group() for memcpy() region iommu/amd: Use struct_group() for memcpy() region cxgb3: Use struct_group() for memcpy() region ip: Use struct_group() for memcpy() regions intersil: Use struct_group() for memcpy() region cxgb4: Use struct_group() for memcpy() region bnx2x: Use struct_group() for memcpy() region drm/amd/pm: Use struct_group() for memcpy() region staging: wlan-ng: Use struct_group() for memcpy() region drm/mga/mga_ioc32: Use struct_group() for memcpy() region net/mlx5e: Use struct_group() for memcpy() region HID: cp2112: Use struct_group() for memcpy() region Prepare fortify for additional hardening: compiler_types.h: Remove __compiletime_object_size() lib/string: Move helper functions out of string.c fortify: Move remaining fortify helpers into fortify-string.h fortify: Explicitly disable Clang support Add compile-time and run-time tests: fortify: Add compile-time FORTIFY_SOURCE tests lib: Introduce CONFIG_TEST_MEMCPY Enable new compile-time memcpy() and memmove() bounds checking: fortify: Detect struct member overflows in memcpy() at compile-time fortify: Detect struct member overflows in memmove() at compile-time Clean up remaining simple compile-time memset() warnings: scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp Introduce memset_after() helper and apply it (and struct_group()) treewide to avoid compile-time memset() warnings: string.h: Introduce memset_after() for wiping trailing members/padding xfrm: Use memset_after() to clear padding mac80211: Use memset_after() to clear tx status net: 802: Use memset_after() to clear struct fields net: dccp: Use memset_after() for TP zeroing net: qede: Use memset_after() for counters ath11k: Use memset_after() for clearing queue descriptors iw_cxgb4: Use memset_after() for cpl_t5_pass_accept_rpl intel_th: msu: Use memset_after() for clearing hw header IB/mthca: Use memset_after() for clearing mpt_entry btrfs: Use memset_after() to clear end of struct drbd: Use struct_group() to zero algs cm4000_cs: Use struct_group() to zero struct cm4000_dev region KVM: x86: Use struct_group() to zero decode cache tracing: Use struct_group() to zero struct trace_iterator dm integrity: Use struct_group() to zero struct journal_sector HID: roccat: Use struct_group() to zero kone_mouse_event ipv6: Use struct_group() to zero rt6_info RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr ethtool: stats: Use struct_group() to clear all stats at once netfilter: conntrack: Use struct_group() to zero struct nf_conn powerpc: Split memset() to avoid multi-field overflow Enable new compile-time memset() bounds checking: fortify: Detect struct member overflows in memset() at compile-time Enable Clang support and global array-bounds checking: fortify: Work around Clang inlining bugs Makefile: Enable -Warray-bounds Avoid run-time memcpy() bounds check warnings: netlink: Avoid false-positive memcpy() warning iwlwifi: dbg_ini: Split memcpy() to avoid multi-field write Enable run-time memcpy() bounds checking: fortify: Add run-time WARN for cross-field memcpy() A future series will clean up for and add run-time memset() bounds checking. Thanks! -Kees Makefile | 1 - arch/s390/lib/string.c | 3 + arch/x86/boot/compressed/misc.c | 3 +- arch/x86/kvm/emulate.c | 3 +- arch/x86/kvm/kvm_emulate.h | 19 +- arch/x86/lib/memcpy_32.c | 1 + arch/x86/lib/string_32.c | 1 + drivers/block/drbd/drbd_main.c | 3 +- drivers/block/drbd/drbd_protocol.h | 6 +- drivers/block/drbd/drbd_receiver.c | 3 +- drivers/char/pcmcia/cm4000_cs.c | 9 +- drivers/gpu/drm/amd/include/atomfirmware.h | 9 +- .../drm/amd/pm/inc/smu11_driver_if_arcturus.h | 3 +- .../drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 +- .../amd/pm/inc/smu13_driver_if_aldebaran.h | 3 +- .../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 +- .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 12 +- .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 +- drivers/gpu/drm/mga/mga_ioc32.c | 30 +- drivers/hid/hid-cp2112.c | 14 +- drivers/hid/hid-roccat-kone.c | 2 +- drivers/hid/hid-roccat-kone.h | 12 +- drivers/hwtracing/intel_th/msu.c | 4 +- drivers/infiniband/hw/cxgb4/cm.c | 5 +- drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 +- drivers/infiniband/hw/mthca/mthca_mr.c | 3 +- drivers/iommu/amd/init.c | 9 +- drivers/macintosh/smu.c | 3 +- drivers/md/dm-integrity.c | 9 +- drivers/media/platform/omap3isp/ispstat.c | 5 +- .../net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 7 +- .../net/ethernet/broadcom/bnx2x/bnx2x_stats.h | 14 +- drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c | 4 +- drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h | 14 +- drivers/net/ethernet/chelsio/cxgb3/sge.c | 9 +- drivers/net/ethernet/chelsio/cxgb4/sge.c | 8 +- drivers/net/ethernet/chelsio/cxgb4/t4_msg.h | 2 +- drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h | 10 +- drivers/net/ethernet/chelsio/cxgb4vf/sge.c | 7 +- drivers/net/ethernet/mellanox/mlx5/core/en.h | 4 +- .../net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 +- .../net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +- drivers/net/ethernet/qlogic/qede/qede_main.c | 2 +- drivers/net/wireguard/queueing.h | 4 +- drivers/net/wireless/ath/ath11k/hal_rx.c | 13 +- drivers/net/wireless/ath/carl9170/tx.c | 4 +- drivers/net/wireless/intel/ipw2x00/libipw.h | 12 +- .../net/wireless/intel/ipw2x00/libipw_rx.c | 8 +- drivers/net/wireless/intel/iwlwifi/fw/file.h | 2 +- .../net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 3 +- .../net/wireless/intersil/hostap/hostap_hw.c | 5 +- .../wireless/intersil/hostap/hostap_wlan.h | 14 +- drivers/net/wireless/intersil/p54/txrx.c | 4 +- drivers/net/wireless/marvell/libertas/host.h | 10 +- drivers/net/wireless/marvell/libertas/tx.c | 5 +- .../marvell/libertas_tf/libertas_tf.h | 10 +- .../net/wireless/marvell/libertas_tf/main.c | 3 +- drivers/net/wireless/marvell/mwl8k.c | 10 +- drivers/rpmsg/qcom_glink_native.c | 2 +- drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- drivers/staging/rtl8192e/rtllib.h | 20 +- drivers/staging/rtl8192e/rtllib_crypt_ccmp.c | 3 +- drivers/staging/rtl8192e/rtllib_rx.c | 8 +- .../staging/rtl8192u/ieee80211/ieee80211.h | 24 +- .../rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 3 +- .../staging/rtl8192u/ieee80211/ieee80211_rx.c | 8 +- drivers/staging/rtl8723bs/core/rtw_mlme.c | 2 +- drivers/staging/rtl8723bs/core/rtw_security.c | 5 +- drivers/staging/rtl8723bs/core/rtw_xmit.c | 5 +- drivers/staging/wlan-ng/hfa384x.h | 16 +- drivers/staging/wlan-ng/hfa384x_usb.c | 4 +- .../intel/int340x_thermal/acpi_thermal_rel.c | 5 +- .../intel/int340x_thermal/acpi_thermal_rel.h | 48 +-- fs/btrfs/root-tree.c | 5 +- include/linux/compiler-gcc.h | 2 - include/linux/compiler_types.h | 4 - include/linux/fortify-string.h | 234 +++++++++++--- include/linux/ieee80211.h | 8 +- include/linux/if_vlan.h | 6 +- include/linux/skbuff.h | 9 +- include/linux/stddef.h | 34 ++ include/linux/string.h | 26 +- include/linux/thread_info.h | 2 +- include/linux/trace_events.h | 26 +- include/net/flow.h | 6 +- include/net/ieee80211_radiotap.h | 24 +- include/net/ip6_fib.h | 30 +- include/net/mac80211.h | 4 +- include/net/netfilter/nf_conntrack.h | 20 +- include/uapi/drm/mga_drm.h | 37 ++- include/uapi/linux/if_ether.h | 12 +- include/uapi/linux/ip.h | 12 +- include/uapi/linux/ipv6.h | 12 +- include/uapi/linux/netlink.h | 1 + include/uapi/linux/omap3isp.h | 44 ++- kernel/trace/trace.c | 4 +- lib/.gitignore | 2 + lib/Kconfig.debug | 3 + lib/Makefile | 32 ++ lib/string.c | 210 +------------ lib/string_helpers.c | 201 ++++++++++++ lib/test_fortify/read_overflow-memchr.c | 5 + lib/test_fortify/read_overflow-memchr_inv.c | 5 + lib/test_fortify/read_overflow-memcmp.c | 5 + lib/test_fortify/read_overflow-memscan.c | 5 + lib/test_fortify/read_overflow2-memcmp.c | 5 + lib/test_fortify/read_overflow2-memcpy.c | 5 + lib/test_fortify/read_overflow2-memmove.c | 5 + .../read_overflow2_field-memcpy.c | 5 + .../read_overflow2_field-memmove.c | 5 + lib/test_fortify/test_fortify.h | 31 ++ lib/test_fortify/write_overflow-memcpy.c | 5 + lib/test_fortify/write_overflow-memmove.c | 5 + lib/test_fortify/write_overflow-memset.c | 5 + lib/test_fortify/write_overflow-strlcpy.c | 5 + lib/test_fortify/write_overflow-strncpy.c | 5 + lib/test_fortify/write_overflow-strscpy.c | 5 + .../write_overflow_field-memcpy.c | 5 + .../write_overflow_field-memmove.c | 5 + .../write_overflow_field-memset.c | 5 + lib/test_memcpy.c | 297 ++++++++++++++++++ net/802/hippi.c | 2 +- net/core/flow_dissector.c | 10 +- net/core/skbuff.c | 14 +- net/dccp/trace.h | 4 +- net/ethtool/stats.c | 15 +- net/ipv4/ip_output.c | 6 +- net/ipv6/route.c | 4 +- net/mac80211/rx.c | 2 +- net/netfilter/nf_conntrack_core.c | 4 +- net/netlink/af_netlink.c | 4 +- net/wireless/lib80211_crypt_ccmp.c | 3 +- net/wireless/radiotap.c | 5 +- net/xfrm/xfrm_policy.c | 4 +- net/xfrm/xfrm_user.c | 2 +- scripts/test_fortify.sh | 64 ++++ security/Kconfig | 3 + 137 files changed, 1484 insertions(+), 633 deletions(-) create mode 100644 lib/test_fortify/read_overflow-memchr.c create mode 100644 lib/test_fortify/read_overflow-memchr_inv.c create mode 100644 lib/test_fortify/read_overflow-memcmp.c create mode 100644 lib/test_fortify/read_overflow-memscan.c create mode 100644 lib/test_fortify/read_overflow2-memcmp.c create mode 100644 lib/test_fortify/read_overflow2-memcpy.c create mode 100644 lib/test_fortify/read_overflow2-memmove.c create mode 100644 lib/test_fortify/read_overflow2_field-memcpy.c create mode 100644 lib/test_fortify/read_overflow2_field-memmove.c create mode 100644 lib/test_fortify/test_fortify.h create mode 100644 lib/test_fortify/write_overflow-memcpy.c create mode 100644 lib/test_fortify/write_overflow-memmove.c create mode 100644 lib/test_fortify/write_overflow-memset.c create mode 100644 lib/test_fortify/write_overflow-strlcpy.c create mode 100644 lib/test_fortify/write_overflow-strncpy.c create mode 100644 lib/test_fortify/write_overflow-strscpy.c create mode 100644 lib/test_fortify/write_overflow_field-memcpy.c create mode 100644 lib/test_fortify/write_overflow_field-memmove.c create mode 100644 lib/test_fortify/write_overflow_field-memset.c create mode 100644 lib/test_memcpy.c create mode 100644 scripts/test_fortify.sh