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

[PATCH net-next v3 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) to prevent such case happens
again.

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

Changes since v2:
1/4 - don't reset the reason and print the debug warning only (Jakub
      Kicinski)
4/4 - remove new lines between tags

Changes since v1:
- consider tcp_v6_rcv() in the 4th patch


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 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()

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

-- 
2.36.1

[PATCH net-next v3 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'.

Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
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..41cac0e4834e 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 (unlikely(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 v3 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() with
DEBUG_NET_WARN_ON_ONCE().

Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v3:
- don't reset the reason and print the debug warning only (Jakub Kicinski)
---
 net/core/skbuff.c | 2 ++
 1 file changed, 2 insertions(+)

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

[PATCH net-next v3 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.

Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
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 b91d225fdc13..4db3f4a33580 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -510,7 +510,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 v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

The 'drop_reason' that passed to kfree_skb_reason() in tcp_v4_rcv()
and tcp_v6_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")
Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v3:
- remove new lines between tags
v2:
- consider tcp_v6_rcv()
---
 net/ipv4/tcp_ipv4.c | 1 +
 net/ipv6/tcp_ipv6.c | 1 +
 2 files changed, 2 insertions(+)

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;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 60bdec257ba7..636ed23d9af0 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1509,6 +1509,7 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 discard:
 	if (opt_skb)
 		__kfree_skb(opt_skb);
+	SKB_DR_OR(reason, NOT_SPECIFIED);
 	kfree_skb_reason(skb, reason);
 	return 0;
 csum_err:
-- 
2.36.1

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 13 May 2022 11:03:35 +0800 you 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.
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/4] net: dm: check the boundary of skb drop reasons
    https://git.kernel.org/netdev/net-next/c/a3af33abd921
  - [net-next,v3,2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason()
    https://git.kernel.org/netdev/net-next/c/20bbcd0a94c6
  - [net-next,v3,3/4] net: skb: change the definition SKB_DR_SET()
    https://git.kernel.org/netdev/net-next/c/7ebd3f3ee51a
  - [net-next,v3,4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()
    https://git.kernel.org/netdev/net-next/c/f8319dfd1b3b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()

[Jakub Kicinski]

On Fri, 13 May 2022 11:03:39 +0800 menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> The 'drop_reason' that passed to kfree_skb_reason() in tcp_v4_rcv()
> and tcp_v6_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")

This patch is in net, should this fix have been targeting net / 5.18?

Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()

[Menglong Dong]

On Thu, May 19, 2022 at 11:48 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 13 May 2022 11:03:39 +0800 menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > The 'drop_reason' that passed to kfree_skb_reason() in tcp_v4_rcv()
> > and tcp_v6_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")
>
> This patch is in net, should this fix have been targeting net / 5.18?

Yeah, I think it should have. What do I need to do? CC someone?

Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()

[Jakub Kicinski]

On Fri, 20 May 2022 09:46:49 +0800 Menglong Dong wrote:
> > This patch is in net, should this fix have been targeting net / 5.18?  
> 
> Yeah, I think it should have. What do I need to do? CC someone?

Too late now, I was just double checking. It can make its way to the
current release via stable in a week or two.

BTW I'm about to send a fixup to patch 4, stay tuned.

Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()

[Jakub Kicinski]

On Thu, 19 May 2022 19:09:15 -0700 Jakub Kicinski wrote:
> On Fri, 20 May 2022 09:46:49 +0800 Menglong Dong wrote:
> > > This patch is in net, should this fix have been targeting net / 5.18?    
> > 
> > Yeah, I think it should have. What do I need to do? CC someone?  
> 
> Too late now, I was just double checking. It can make its way to the
> current release via stable in a week or two.

Ah, FWIW my initial question was missing "-next" - I meant to say that
the patch is in net-next rather than net. I think you got what I meant..

Re: [PATCH net-next v3 4/4] net: tcp: reset 'drop_reason' to NOT_SPCIFIED in tcp_v{4,6}_rcv()

[Menglong Dong]

On Fri, May 20, 2022 at 10:18 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 19 May 2022 19:09:15 -0700 Jakub Kicinski wrote:
> > On Fri, 20 May 2022 09:46:49 +0800 Menglong Dong wrote:
> > > > This patch is in net, should this fix have been targeting net / 5.18?
> > >
> > > Yeah, I think it should have. What do I need to do? CC someone?
> >
> > Too late now, I was just double checking. It can make its way to the
> > current release via stable in a week or two.
>
> Ah, FWIW my initial question was missing "-next" - I meant to say that
> the patch is in net-next rather than net. I think you got what I meant..

Yeah, I get what you mean now. Such bug-fix patches should target 'net'
rather than 'net-next'.

BTW, thanks for your fixup...I am still surprised at my mistake.