lkml.org 
[lkml]   [2021]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 19/64] ip: Use struct_group() for memcpy() regions
    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

    \
     
     \ /
      Last update: 2021-07-28 07:58    [W:4.145 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site