| Message ID | 20210727205855.411487-6-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
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 >
diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h index 4ef2944a68bc..52da5e963003 100644 --- a/drivers/net/wireguard/queueing.h +++ b/drivers/net/wireguard/queueing.h @@ -79,9 +79,7 @@ static inline void wg_reset_packet(struct sk_buff *skb, bool encapsulating) u8 sw_hash = skb->sw_hash; u32 hash = skb->hash; skb_scrub_packet(skb, true); - memset(&skb->headers_start, 0, - offsetof(struct sk_buff, headers_end) - - offsetof(struct sk_buff, headers_start)); + memset(&skb->headers, 0, sizeof(skb->headers)); if (encapsulating) { skb->l4_hash = l4_hash; skb->sw_hash = sw_hash; diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index f19190820e63..b4032e9b130e 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -800,11 +800,10 @@ struct sk_buff { __u8 active_extensions; #endif - /* fields enclosed in headers_start/headers_end are copied + /* Fields enclosed in headers group are copied * using a single memcpy() in __copy_skb_header() */ - /* private: */ - __u32 headers_start[0]; + struct_group(headers, /* public: */ /* if you move pkt_type around you also must adapt those constants */ @@ -920,8 +919,8 @@ struct sk_buff { u64 kcov_handle; #endif - /* private: */ - __u32 headers_end[0]; + ); /* end headers group */ + /* public: */ /* These elements must be at the end, see alloc_skb() for details. */ diff --git a/net/core/skbuff.c b/net/core/skbuff.c index fc7942c0dddc..5f29c65507e0 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -987,12 +987,10 @@ void napi_consume_skb(struct sk_buff *skb, int budget) } EXPORT_SYMBOL(napi_consume_skb); -/* Make sure a field is enclosed inside headers_start/headers_end section */ +/* Make sure a field is contained by headers group */ #define CHECK_SKB_FIELD(field) \ - BUILD_BUG_ON(offsetof(struct sk_buff, field) < \ - offsetof(struct sk_buff, headers_start)); \ - BUILD_BUG_ON(offsetof(struct sk_buff, field) > \ - offsetof(struct sk_buff, headers_end)); \ + BUILD_BUG_ON(offsetof(struct sk_buff, field) != \ + offsetof(struct sk_buff, headers.field)); \ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) { @@ -1004,14 +1002,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old) __skb_ext_copy(new, old); __nf_copy(new, old, false); - /* Note : this field could be in headers_start/headers_end section + /* Note : this field could be in the headers group. * It is not yet because we do not want to have a 16 bit hole */ new->queue_mapping = old->queue_mapping; - memcpy(&new->headers_start, &old->headers_start, - offsetof(struct sk_buff, headers_end) - - offsetof(struct sk_buff, headers_start)); + memcpy(&new->headers, &old->headers, sizeof(new->headers)); CHECK_SKB_FIELD(protocol); CHECK_SKB_FIELD(csum); CHECK_SKB_FIELD(hash);
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memcpy(), memmove(), and memset(), avoid intentionally writing across neighboring fields. Replace the existing empty member position markers "headers_start" and "headers_end" with a struct_group(). This will allow memcpy() and sizeof() to more easily reason about sizes, and improve readability. "pahole" shows no size nor member offset changes to struct sk_buff. "objdump -d" shows no no meaningful object code changes (i.e. only source line number induced differences and optimizations.) Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/net/wireguard/queueing.h | 4 +--- include/linux/skbuff.h | 9 ++++----- net/core/skbuff.c | 14 +++++--------- 3 files changed, 10 insertions(+), 17 deletions(-)