| Message ID | 20220509165716.745111-1-eric.dumazet@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: warn if transport header was not set | expand |
On Mon, 9 May 2022 09:57:16 -0700 Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > Make sure skb_transport_header() and skb_transport_offset() uses > are not fooled if the transport header has not been set. > > This change will likely expose existing bugs in linux networking stacks. > > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > include/linux/skbuff.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index d58669d6cb91aa30edc70d59a0a7e9d4e2298842..043c59fa0bd6d921f2d2e211348929681bfce186 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -2804,6 +2804,7 @@ static inline bool skb_transport_header_was_set(const struct sk_buff *skb) > > static inline unsigned char *skb_transport_header(const struct sk_buff *skb) > { > + WARN_ON_ONCE(!skb_transport_header_was_set(skb)); > return skb->head + skb->transport_header; > } > This is for prod or for syzbot? What are your feelings on putting this under a kconfig? We already have a #ifdef DEBUG in skb_checksum_none_assert() we could generalize that into some form of kconfig-gated SKB_CHECK(). Kconfig-gated so that people don't feel self-conscious about using it. I wrote such a patch a few times already but never sent it.
On Mon, May 9, 2022 at 10:21 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 9 May 2022 09:57:16 -0700 Eric Dumazet wrote: > > From: Eric Dumazet <edumazet@google.com> > > > > Make sure skb_transport_header() and skb_transport_offset() uses > > are not fooled if the transport header has not been set. > > > > This change will likely expose existing bugs in linux networking stacks. > > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > --- > > include/linux/skbuff.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index d58669d6cb91aa30edc70d59a0a7e9d4e2298842..043c59fa0bd6d921f2d2e211348929681bfce186 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -2804,6 +2804,7 @@ static inline bool skb_transport_header_was_set(const struct sk_buff *skb) > > > > static inline unsigned char *skb_transport_header(const struct sk_buff *skb) > > { > > + WARN_ON_ONCE(!skb_transport_header_was_set(skb)); > > return skb->head + skb->transport_header; > > } > > > > This is for prod or for syzbot? > > What are your feelings on putting this under a kconfig? > > We already have a #ifdef DEBUG in skb_checksum_none_assert() > we could generalize that into some form of kconfig-gated > SKB_CHECK(). Kconfig-gated so that people don't feel self-conscious > about using it. I wrote such a patch a few times already but never > sent it. Sure, we can add a CONFIG_DEBUG_NET, I think it has been suggested in the past. Then we can ask syzbot teams to turn on this option. I will send a series with this.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index d58669d6cb91aa30edc70d59a0a7e9d4e2298842..043c59fa0bd6d921f2d2e211348929681bfce186 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -2804,6 +2804,7 @@ static inline bool skb_transport_header_was_set(const struct sk_buff *skb) static inline unsigned char *skb_transport_header(const struct sk_buff *skb) { + WARN_ON_ONCE(!skb_transport_header_was_set(skb)); return skb->head + skb->transport_header; }