| Message ID | 20210727205855.411487-1-keescook@chromium.org |
|---|---|
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
On 7/27/21 4:31 PM, Bart Van Assche wrote: > On 7/27/21 1:58 PM, Kees Cook wrote: >> +static int __init test_memcpy_init(void) >> +{ >> + int err = 0; >> + >> + err |= test_memcpy(); >> + err |= test_memmove(); >> + err |= test_memset(); >> + >> + if (err) { >> + pr_warn("FAIL!\n"); >> + err = -EINVAL; >> + } else { >> + pr_info("all tests passed\n"); >> + } >> + >> + return err; >> +} >> + >> +static void __exit test_memcpy_exit(void) >> +{ } >> + >> +module_init(test_memcpy_init); >> +module_exit(test_memcpy_exit); >> +MODULE_LICENSE("GPL"); > > Has it been considered to implement this test using the Kunit framework? and do we want everything converted to use the Kunit test framework? My answer is No, we don't, but I could easily be in the minority.
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 >
On Tue, Jul 27, 2021 at 04:31:03PM -0700, Bart Van Assche wrote: > On 7/27/21 1:58 PM, Kees Cook wrote: > > +static int __init test_memcpy_init(void) > > +{ > > + int err = 0; > > + > > + err |= test_memcpy(); > > + err |= test_memmove(); > > + err |= test_memset(); > > + > > + if (err) { > > + pr_warn("FAIL!\n"); > > + err = -EINVAL; > > + } else { > > + pr_info("all tests passed\n"); > > + } > > + > > + return err; > > +} > > + > > +static void __exit test_memcpy_exit(void) > > +{ } > > + > > +module_init(test_memcpy_init); > > +module_exit(test_memcpy_exit); > > +MODULE_LICENSE("GPL"); > > Has it been considered to implement this test using the Kunit framework? Good point! I will see if that works here; it would make sense to make this KUnit from the start.
On Tue, Jul 27, 2021 at 01:58:00PM -0700, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), memmove(), and memset(), avoid > intentionally writing across neighboring fields. > > Adjust memcpy() destination to be the named structure itself, rather than > the first member, allowing memcpy() to correctly reason about the size. > > "objdump -d" shows no object code changes. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/staging/rtl8723bs/core/rtw_mlme.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) 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 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: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: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 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 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 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. -- 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; -- Kees Cook
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 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
On Thu, Jul 29, 2021 at 11:00:48PM -0700, Kees Cook wrote: > 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. Then is explicit memset the only reliable way accross all compiler flavors and supported versions? E.g. for ioctls that get kernel memory (stack, kmalloc), partially initialize it and then call copy_to_user.
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 Fri, Jul 30, 2021 at 10:08:03AM -0700, Nick Desaulniers wrote: > On Fri, Jul 30, 2021 at 9:44 AM Kees Cook <keescook@chromium.org> wrote: > > > > On Fri, Jul 30, 2021 at 12:00:54PM +0300, Dan Carpenter wrote: > > > On Fri, Jul 30, 2021 at 10:38:45AM +0200, David Sterba wrote: > > > > Then is explicit memset the only reliable way accross all compiler > > > > flavors and supported versions? > > > > > > > > > > The = { } initializer works. It's only when you start partially > > > initializing the struct that it doesn't initialize holes. > > > > No, partial works. It's when you _fully_ initialize the struct where the > > padding doesn't get initialized. *sob* > > I'm pretty sure that this has more to do with whether or not the > compiler applies SROA then observes uses of the individual members or > not. Ultimately, it's just not consistent, so thank goodness for -ftrivial-auto-var-init=zero. :) -- Kees Cook
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