[PATCH net-next 0/4] net: skb: check the boundrary of skb drop reason

[PATCH net-next 0/4] net: skb: check the boundrary of skb drop reason

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

In the commit 1330b6ef3313 ("skb: make drop reason booleanable"),
SKB_NOT_DROPPED_YET is added to the enum skb_drop_reason, which makes
the invalid drop reason SKB_NOT_DROPPED_YET can leak to the kfree_skb
tracepoint. Once this happen (it happened, as 4th patch says), it can
cause NULL pointer in drop monitor and result in kernel panic.

Therefore, check the boundrary of drop reason in both kfree_skb_reason
(2th patch) and drop monitor (1th patch).

Meanwhile, fix the invalid drop reason passed to kfree_skb_reason() in
tcp_v4_rcv().

Menglong Dong (4):
  net: dm: check the boundary of skb drop reasons
  net: skb: check the boundrary of drop reason in kfree_skb_reason()
  net: skb: change the definition SKB_DR_SET()
  net: tcp: reset skb drop reason to NOT_SPCIFIED in tcp_v4_rcv()

 include/linux/skbuff.h  | 3 ++-
 net/core/drop_monitor.c | 2 +-
 net/core/skbuff.c       | 5 +++++
 net/ipv4/tcp_ipv4.c     | 1 +
 4 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.36.1

[PATCH net-next 1/4] net: dm: check the boundary of skb drop reasons

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

The 'reason' will be set to 'SKB_DROP_REASON_NOT_SPECIFIED' if it not
small that SKB_DROP_REASON_MAX in net_dm_packet_trace_kfree_skb_hit(),
but it can't avoid it to be 0, which is invalid and can cause NULL
pointer in drop_reasons.

Therefore, reset it to SKB_DROP_REASON_NOT_SPECIFIED when 'reason <= 0'.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 net/core/drop_monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index b89e3e95bffc..020ac5f214d0 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -517,7 +517,7 @@ static void net_dm_packet_trace_kfree_skb_hit(void *ignore,
 	if (!nskb)
 		return;
 
-	if ((unsigned int)reason >= SKB_DROP_REASON_MAX)
+	if (reason >= SKB_DROP_REASON_MAX || reason <= 0)
 		reason = SKB_DROP_REASON_NOT_SPECIFIED;
 	cb = NET_DM_SKB_CB(nskb);
 	cb->reason = reason;
-- 
2.36.1

[PATCH net-next 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Sometimes, we may forget to reset skb drop reason to NOT_SPECIFIED after
we make it the return value of the functions with return type of enum
skb_drop_reason, such as tcp_inbound_md5_hash. Therefore, its value can
be SKB_NOT_DROPPED_YET(0), which is invalid for kfree_skb_reason().

So we check the range of drop reason in kfree_skb_reason() and reset it
to NOT_SPECIFIED and print a warning with DEBUG_NET_WARN_ON_ONCE() if it
is invalid.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 net/core/skbuff.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 15f7b6f99a8f..e49e43d4c34d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -771,6 +771,11 @@ void kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
 	if (!skb_unref(skb))
 		return;
 
+	if (unlikely(reason <= 0 || reason >= SKB_DROP_REASON_MAX)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		reason = SKB_DROP_REASON_NOT_SPECIFIED;
+	}
+
 	trace_kfree_skb(skb, __builtin_return_address(0), reason);
 	__kfree_skb(skb);
 }
-- 
2.36.1

[PATCH net-next 3/4] net: skb: change the definition SKB_DR_SET()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

The SKB_DR_OR() is used to set the drop reason to a value when it is
not set or specified yet. SKB_NOT_DROPPED_YET should also be considered
as not set.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 5c2599e3fe7d..818347b0180e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -480,7 +480,8 @@ enum skb_drop_reason {
 	(name = SKB_DROP_REASON_##reason)
 #define SKB_DR_OR(name, reason)					\
 	do {							\
-		if (name == SKB_DROP_REASON_NOT_SPECIFIED)	\
+		if (name == SKB_DROP_REASON_NOT_SPECIFIED ||	\
+		    name == SKB_NOT_DROPPED_YET)		\
 			SKB_DR_SET(name, reason);		\
 	} while (0)
 
-- 
2.36.1

[PATCH net-next 4/4] net: tcp: reset skb drop reason to NOT_SPCIFIED in tcp_v4_rcv()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

The 'drop_reason' that passed to kfree_skb_reason() in tcp_v4_rcv()
can be SKB_NOT_DROPPED_YET(0), as it is used as the return value of
tcp_inbound_md5_hash().

And it can panic the kernel with NULL pointer in
net_dm_packet_report_size() if the reason is 0, as drop_reasons[0]
is NULL.

Fixes: 1330b6ef3313 ("skb: make drop reason booleanable")

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 net/ipv4/tcp_ipv4.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 918816ec5dd4..24eb42497a71 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2101,6 +2101,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 	}
 
 discard_it:
+	SKB_DR_OR(drop_reason, NOT_SPECIFIED);
 	/* Discard frame. */
 	kfree_skb_reason(skb, drop_reason);
 	return 0;
-- 
2.36.1

Re: [PATCH net-next 0/4] net: skb: check the boundrary of skb drop reason

[Menglong Dong]

On Thu, May 12, 2022 at 2:26 PM <menglong8.dong@gmail.com> wrote:
>
> From: Menglong Dong <imagedong@tencent.com>
>
> In the commit 1330b6ef3313 ("skb: make drop reason booleanable"),
> SKB_NOT_DROPPED_YET is added to the enum skb_drop_reason, which makes
> the invalid drop reason SKB_NOT_DROPPED_YET can leak to the kfree_skb
> tracepoint. Once this happen (it happened, as 4th patch says), it can
> cause NULL pointer in drop monitor and result in kernel panic.
>
> Therefore, check the boundrary of drop reason in both kfree_skb_reason
> (2th patch) and drop monitor (1th patch).
>
> Meanwhile, fix the invalid drop reason passed to kfree_skb_reason() in
> tcp_v4_rcv().
>

tcp_v6_rcv() is forgeted, I'll send a V2 :/

> Menglong Dong (4):
>   net: dm: check the boundary of skb drop reasons
>   net: skb: check the boundrary of drop reason in kfree_skb_reason()
>   net: skb: change the definition SKB_DR_SET()
>   net: tcp: reset skb drop reason to NOT_SPCIFIED in tcp_v4_rcv()
>
>  include/linux/skbuff.h  | 3 ++-
>  net/core/drop_monitor.c | 2 +-
>  net/core/skbuff.c       | 5 +++++
>  net/ipv4/tcp_ipv4.c     | 1 +
>  4 files changed, 9 insertions(+), 2 deletions(-)
>
> --
> 2.36.1
>

Re: [PATCH net-next 0/4] net: skb: check the boundrary of skb drop reason

[Jakub Kicinski]

On Thu, 12 May 2022 20:31:14 +0800 Menglong Dong wrote:
> On Thu, May 12, 2022 at 2:26 PM <menglong8.dong@gmail.com> wrote:
> >
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > In the commit 1330b6ef3313 ("skb: make drop reason booleanable"),
> > SKB_NOT_DROPPED_YET is added to the enum skb_drop_reason, which makes
> > the invalid drop reason SKB_NOT_DROPPED_YET can leak to the kfree_skb
> > tracepoint. Once this happen (it happened, as 4th patch says), it can
> > cause NULL pointer in drop monitor and result in kernel panic.
> >
> > Therefore, check the boundrary of drop reason in both kfree_skb_reason
> > (2th patch) and drop monitor (1th patch).
> >
> > Meanwhile, fix the invalid drop reason passed to kfree_skb_reason() in
> > tcp_v4_rcv().
> >  
> 
> tcp_v6_rcv() is forgeted, I'll send a V2 :/

Please don't repost stuff within 24h:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#i-have-received-review-feedback-when-should-i-post-a-revised-version-of-the-patches

I could have given you the same exact feedback on v1 as v2...