| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 28 Jul 2021 01:37:33 -0500
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Kees Cook <keescook@...omium.org>, linux-hardening@...r.kernel.org,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
Keith Packard <keithpac@...zon.com>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
netdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
linux-staging@...ts.linux.dev, linux-block@...r.kernel.org,
linux-kbuild@...r.kernel.org, clang-built-linux@...glegroups.com
Subject: Re: [PATCH 19/64] ip: Use struct_group() for memcpy() regions
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@...omium.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
Powered by blists - more mailing lists