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

[PATCH net-next v2 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 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       | 5 +++++
 net/ipv4/tcp_ipv4.c     | 1 +
 net/ipv6/tcp_ipv6.c     | 1 +
 5 files changed, 10 insertions(+), 2 deletions(-)

-- 
2.36.1

[PATCH net-next v2 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..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 v2 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 v2 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 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 v2 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")

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
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 v2 2/4] net: skb: check the boundrary of drop reason in kfree_skb_reason()

[Jakub Kicinski]

On Thu, 12 May 2022 20:33:11 +0800 menglong8.dong@gmail.com wrote:
> +	if (unlikely(reason <= 0 || reason >= SKB_DROP_REASON_MAX)) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		reason = SKB_DROP_REASON_NOT_SPECIFIED;
> +	}

With drop_monitor fixes sending an invalid reason to the tracepoint
should be a minor bug, right?

Can we just have a:

	DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX);

and avoid having this branch on non-debug builds?

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

[Eric Dumazet]

On Thu, May 12, 2022 at 9:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 12 May 2022 20:33:11 +0800 menglong8.dong@gmail.com wrote:
> > +     if (unlikely(reason <= 0 || reason >= SKB_DROP_REASON_MAX)) {
> > +             DEBUG_NET_WARN_ON_ONCE(1);
> > +             reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > +     }
>
> With drop_monitor fixes sending an invalid reason to the tracepoint
> should be a minor bug, right?
>
> Can we just have a:
>
>         DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX);
>
> and avoid having this branch on non-debug builds?

Exactly what I was going to say.

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

[Jakub Kicinski]

On Thu, 12 May 2022 20:33:13 +0800 menglong8.dong@gmail.com wrote:
> Fixes: 1330b6ef3313 ("skb: make drop reason booleanable")
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>

No new lines between tags

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

[Menglong Dong]

On Fri, May 13, 2022 at 12:16 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 12 May 2022 20:33:11 +0800 menglong8.dong@gmail.com wrote:
> > +     if (unlikely(reason <= 0 || reason >= SKB_DROP_REASON_MAX)) {
> > +             DEBUG_NET_WARN_ON_ONCE(1);
> > +             reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > +     }
>
> With drop_monitor fixes sending an invalid reason to the tracepoint
> should be a minor bug, right?
>
> Can we just have a:
>
>         DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX);
>
> and avoid having this branch on non-debug builds?

Yeah, it seems this way is more logical. I'll change it in the V3.