diff mbox series

[2/2] skbuff: Extract list pointers to silence compiler warnings

Message ID 20220329220256.72283-2-tadeusz.struk@linaro.org
State New
Headers show
Series None | expand

Commit Message

Tadeusz Struk March 29, 2022, 10:02 p.m. UTC 
Please apply this to stable 5.10.y, and 5.15.y
---8<---

From: Kees Cook <keescook@chromium.org>

Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")

Under both -Warray-bounds and the object_size sanitizer, the compiler is
upset about accessing prev/next of sk_buff when the object it thinks it
is coming from is sk_buff_head. The warning is a false positive due to
the compiler taking a conservative approach, opting to warn at casting
time rather than access time.

However, in support of enabling -Warray-bounds globally (which has
found many real bugs), arrange things for sk_buff so that the compiler
can unambiguously see that there is no intention to access anything
except prev/next.  Introduce and cast to a separate struct sk_buff_list,
which contains _only_ the first two fields, silencing the warnings:

In file included from ./include/net/net_namespace.h:39,
                 from ./include/linux/netdevice.h:37,
                 from net/core/netpoll.c:17:
net/core/netpoll.c: In function 'refill_skbs':
./include/linux/skbuff.h:2086:9: warning: array subscript 'struct sk_buff[0]' is partly outside array bounds of 'struct sk_buff_head[1]' [-Warray-bounds]
 2086 |         __skb_insert(newsk, next->prev, next, list);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/core/netpoll.c:49:28: note: while referencing 'skb_pool'
   49 | static struct sk_buff_head skb_pool;
      |                            ^~~~~~~~

This also upsets UBSAN, which throws a runtime object-size-mismatch
error complaining about skbuff queue helpers, as below, when the kernel
is built with clang and -fsanitize=undefined flag set:

UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2023:28
member access within address ffffc90000cb71c0 with insufficient space
for an object of type 'struct sk_buff'

This change results in no executable instruction differences.

Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20211207062758.2324338-1-keescook@chromium.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org>
---
 include/linux/skbuff.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Greg KH March 30, 2022, 3:29 p.m. UTC | #1 
On Wed, Mar 30, 2022 at 07:59:57AM -0700, Tadeusz Struk wrote:
> On 3/30/22 07:46, Greg KH wrote:
> > On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
> > > Please apply this to stable 5.10.y, and 5.15.y
> > > ---8<---
> > > 
> > > From: Kees Cook<keescook@chromium.org>
> > > 
> > > Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
> > > 
> > > Under both -Warray-bounds and the object_size sanitizer, the compiler is
> > > upset about accessing prev/next of sk_buff when the object it thinks it
> > > is coming from is sk_buff_head. The warning is a false positive due to
> > > the compiler taking a conservative approach, opting to warn at casting
> > > time rather than access time.
> > > 
> > > However, in support of enabling -Warray-bounds globally (which has
> > > found many real bugs), arrange things for sk_buff so that the compiler
> > > can unambiguously see that there is no intention to access anything
> > > except prev/next.  Introduce and cast to a separate struct sk_buff_list,
> > > which contains_only_  the first two fields, silencing the warnings:
> > We don't have -Warray-bounds enabled on any stable kernel tree, so why
> > is this needed?
> > 
> > Where is this showing up as a problem?
> 
> The issue shows up and hinders testing stable kernels in test automations
> like syzkaller:
> 
> https://syzkaller.appspot.com/text?tag=Error&x=12b3aac3700000
> 
> Applying it to stable would enable more test coverage.

Ok, again, that was not obvious, it seemed like you only needed this for
build warnings.

thanks,

greg k-h
Greg KH March 30, 2022, 4:37 p.m. UTC | #2 
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
> Please apply this to stable 5.10.y, and 5.15.y
> ---8<---
> 
> From: Kees Cook <keescook@chromium.org>
> 
> Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")

What about 5.16?
Tadeusz Struk March 30, 2022, 5:10 p.m. UTC | #3 
On 3/30/22 09:37, Greg KH wrote:
> On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
>> Please apply this to stable 5.10.y, and 5.15.y
>> ---8<---
>>
>> From: Kees Cook <keescook@chromium.org>
>>
>> Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
> 
> What about 5.16?
> 
The first one is already in 5.16. The second one applies cleanly, and the build
looks ok.
diff mbox series

Patch

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index acbf1875ad50..b7de22193ec8 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -289,9 +289,11 @@  struct tc_skb_ext {
 #endif
 
 struct sk_buff_head {
+	struct_group_tagged(sk_buff_list, list,
 	/* These two members must be first. */
 	struct sk_buff	*next;
 	struct sk_buff	*prev;
+	);
 
 	__u32		qlen;
 	spinlock_t	lock;
@@ -1906,8 +1908,8 @@  static inline void __skb_insert(struct sk_buff *newsk,
 	 */
 	WRITE_ONCE(newsk->next, next);
 	WRITE_ONCE(newsk->prev, prev);
-	WRITE_ONCE(next->prev, newsk);
-	WRITE_ONCE(prev->next, newsk);
+	WRITE_ONCE(((struct sk_buff_list *)next)->prev, newsk);
+	WRITE_ONCE(((struct sk_buff_list *)prev)->next, newsk);
 	WRITE_ONCE(list->qlen, list->qlen + 1);
 }
 
@@ -2003,7 +2005,7 @@  static inline void __skb_queue_after(struct sk_buff_head *list,
 				     struct sk_buff *prev,
 				     struct sk_buff *newsk)
 {
-	__skb_insert(newsk, prev, prev->next, list);
+	__skb_insert(newsk, prev, ((struct sk_buff_list *)prev)->next, list);
 }
 
 void skb_append(struct sk_buff *old, struct sk_buff *newsk,
@@ -2013,7 +2015,7 @@  static inline void __skb_queue_before(struct sk_buff_head *list,
 				      struct sk_buff *next,
 				      struct sk_buff *newsk)
 {
-	__skb_insert(newsk, next->prev, next, list);
+	__skb_insert(newsk, ((struct sk_buff_list *)next)->prev, next, list);
 }
 
 /**