[PATCH net 3/3] netfilter: flowtable: validate vlan header

[PATCH net 3/3] netfilter: flowtable: validate vlan header

[Pablo Neira Ayuso]

Ensure there is sufficient room to access the protocol field of the
VLAN header, validate it once before the flowtable lookup.

=====================================================
BUG: KMSAN: uninit-value in nf_flow_offload_inet_hook+0x45a/0x5f0 net/netfilter/nf_flow_table_inet.c:32
 nf_flow_offload_inet_hook+0x45a/0x5f0 net/netfilter/nf_flow_table_inet.c:32
 nf_hook_entry_hookfn include/linux/netfilter.h:154 [inline]
 nf_hook_slow+0xf4/0x400 net/netfilter/core.c:626
 nf_hook_ingress include/linux/netfilter_netdev.h:34 [inline]
 nf_ingress net/core/dev.c:5440 [inline]

Fixes: 4cd91f7c290f ("netfilter: flowtable: add vlan support")
Reported-by: syzbot+8407d9bb88cd4c6bf61a@syzkaller.appspotmail.com
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_inet.c | 3 +++
 net/netfilter/nf_flow_table_ip.c   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/net/netfilter/nf_flow_table_inet.c b/net/netfilter/nf_flow_table_inet.c
index 88787b45e30d..dd9a392052ee 100644
--- a/net/netfilter/nf_flow_table_inet.c
+++ b/net/netfilter/nf_flow_table_inet.c
@@ -17,6 +17,9 @@ nf_flow_offload_inet_hook(void *priv, struct sk_buff *skb,
 
 	switch (skb->protocol) {
 	case htons(ETH_P_8021Q):
+		if (!pskb_may_pull(skb, VLAN_HLEN))
+			return NF_ACCEPT;
+
 		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
 		proto = veth->h_vlan_encapsulated_proto;
 		break;
diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index c2c005234dcd..9a9031e9ea1c 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -281,6 +281,9 @@ static bool nf_flow_skb_encap_protocol(struct sk_buff *skb, __be16 proto,
 
 	switch (skb->protocol) {
 	case htons(ETH_P_8021Q):
+		if (!pskb_may_pull(skb, VLAN_HLEN))
+			return false;
+
 		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
 		if (veth->h_vlan_encapsulated_proto == proto) {
 			*offset += VLAN_HLEN;
-- 
2.30.2

Re: [PATCH net 3/3] netfilter: flowtable: validate vlan header

[Eric Dumazet]

On Thu, Aug 22, 2024 at 2:17 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Ensure there is sufficient room to access the protocol field of the
> VLAN header, validate it once before the flowtable lookup.
>
> =====================================================
> BUG: KMSAN: uninit-value in nf_flow_offload_inet_hook+0x45a/0x5f0 net/netfilter/nf_flow_table_inet.c:32
>  nf_flow_offload_inet_hook+0x45a/0x5f0 net/netfilter/nf_flow_table_inet.c:32
>  nf_hook_entry_hookfn include/linux/netfilter.h:154 [inline]
>  nf_hook_slow+0xf4/0x400 net/netfilter/core.c:626
>  nf_hook_ingress include/linux/netfilter_netdev.h:34 [inline]
>  nf_ingress net/core/dev.c:5440 [inline]
>
> Fixes: 4cd91f7c290f ("netfilter: flowtable: add vlan support")
> Reported-by: syzbot+8407d9bb88cd4c6bf61a@syzkaller.appspotmail.com
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_flow_table_inet.c | 3 +++
>  net/netfilter/nf_flow_table_ip.c   | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/net/netfilter/nf_flow_table_inet.c b/net/netfilter/nf_flow_table_inet.c
> index 88787b45e30d..dd9a392052ee 100644
> --- a/net/netfilter/nf_flow_table_inet.c
> +++ b/net/netfilter/nf_flow_table_inet.c
> @@ -17,6 +17,9 @@ nf_flow_offload_inet_hook(void *priv, struct sk_buff *skb,
>
>         switch (skb->protocol) {
>         case htons(ETH_P_8021Q):
> +               if (!pskb_may_pull(skb, VLAN_HLEN))
> +                       return NF_ACCEPT;
> +
>                 veth = (struct vlan_ethhdr *)skb_mac_header(skb);

Is skb_mac_header(skb) always pointing at skb->data - 14 at this stage ?

Otherwise, using

  if (!pskb_may_pull(skb, skb_mac_offset(skb) + sizeof(*veth))  would be safer.

[PATCH net,v2 0/3] Netfilter fixes for net

[Pablo Neira Ayuso]

v2: including suggestion from Eric Dumazet on patch #3.

-o-

Hi,

The following patchset contains Netfilter fixes for net:

Patch #1 disable BH when collecting stats via hardware offload to ensure
         concurrent updates from packet path do not result in losing stats.
         From Sebastian Andrzej Siewior.

Patch #2 uses write seqcount to reset counters serialize against reader.
         Also from Sebastian Andrzej Siewior.

Patch #3 ensures vlan header is in place before accessing its fields,
         according to KMSAN splat triggered by syzbot.

Please, pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git nf-24-08-22

Thanks.

----------------------------------------------------------------

The following changes since commit 807067bf014d4a3ae2cc55bd3de16f22a01eb580:

  kcm: Serialise kcm_sendmsg() for the same socket. (2024-08-19 18:36:12 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git tags/nf-24-08-22

for you to fetch changes up to 6ea14ccb60c8ab829349979b22b58a941ec4a3ee:

  netfilter: flowtable: validate vlan header (2024-08-22 12:14:18 +0200)

----------------------------------------------------------------
netfilter pull request 24-08-22

----------------------------------------------------------------
Pablo Neira Ayuso (1):
      netfilter: flowtable: validate vlan header

Sebastian Andrzej Siewior (2):
      netfilter: nft_counter: Disable BH in nft_counter_offload_stats().
      netfilter: nft_counter: Synchronize nft_counter_reset() against reader.

 net/netfilter/nf_flow_table_inet.c | 3 +++
 net/netfilter/nf_flow_table_ip.c   | 3 +++
 net/netfilter/nft_counter.c        | 9 +++++++--
 3 files changed, 13 insertions(+), 2 deletions(-)

[PATCH net 1/3] netfilter: nft_counter: Disable BH in nft_counter_offload_stats().

[Pablo Neira Ayuso]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

The sequence counter nft_counter_seq is a per-CPU counter. There is no
lock associated with it. nft_counter_do_eval() is using the same counter
and disables BH which suggest that it can be invoked from a softirq.
This in turn means that nft_counter_offload_stats(), which disables only
preemption, can be interrupted by nft_counter_do_eval() leading to two
writer for one seqcount_t.
This can lead to loosing stats or reading statistics while they are
updated.

Disable BH during stats update in nft_counter_offload_stats() to ensure
one writer at a time.

Fixes: b72920f6e4a9d ("netfilter: nftables: counter hardware offload support")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Reviewed-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_counter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nft_counter.c b/net/netfilter/nft_counter.c
index 291ed2026367..16f40b503d37 100644
--- a/net/netfilter/nft_counter.c
+++ b/net/netfilter/nft_counter.c
@@ -265,7 +265,7 @@ static void nft_counter_offload_stats(struct nft_expr *expr,
 	struct nft_counter *this_cpu;
 	seqcount_t *myseq;
 
-	preempt_disable();
+	local_bh_disable();
 	this_cpu = this_cpu_ptr(priv->counter);
 	myseq = this_cpu_ptr(&nft_counter_seq);
 
@@ -273,7 +273,7 @@ static void nft_counter_offload_stats(struct nft_expr *expr,
 	this_cpu->packets += stats->pkts;
 	this_cpu->bytes += stats->bytes;
 	write_seqcount_end(myseq);
-	preempt_enable();
+	local_bh_enable();
 }
 
 void nft_counter_init_seqcount(void)
-- 
2.30.2

[PATCH net 2/3] netfilter: nft_counter: Synchronize nft_counter_reset() against reader.

[Pablo Neira Ayuso]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

nft_counter_reset() resets the counter by subtracting the previously
retrieved value from the counter. This is a write operation on the
counter and as such it requires to be performed with a write sequence of
nft_counter_seq to serialize against its possible reader.

Update the packets/ bytes within write-sequence of nft_counter_seq.

Fixes: d84701ecbcd6a ("netfilter: nft_counter: rework atomic dump and reset")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Reviewed-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_counter.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nft_counter.c b/net/netfilter/nft_counter.c
index 16f40b503d37..eab0dc66bee6 100644
--- a/net/netfilter/nft_counter.c
+++ b/net/netfilter/nft_counter.c
@@ -107,11 +107,16 @@ static void nft_counter_reset(struct nft_counter_percpu_priv *priv,
 			      struct nft_counter *total)
 {
 	struct nft_counter *this_cpu;
+	seqcount_t *myseq;
 
 	local_bh_disable();
 	this_cpu = this_cpu_ptr(priv->counter);
+	myseq = this_cpu_ptr(&nft_counter_seq);
+
+	write_seqcount_begin(myseq);
 	this_cpu->packets -= total->packets;
 	this_cpu->bytes -= total->bytes;
+	write_seqcount_end(myseq);
 	local_bh_enable();
 }
 
-- 
2.30.2

[PATCH net 3/3] netfilter: flowtable: validate vlan header

[Pablo Neira Ayuso]

Ensure there is sufficient room to access the protocol field of the
VLAN header, validate it once before the flowtable lookup.

=====================================================
BUG: KMSAN: uninit-value in nf_flow_offload_inet_hook+0x45a/0x5f0 net/netfilter/nf_flow_table_inet.c:32
 nf_flow_offload_inet_hook+0x45a/0x5f0 net/netfilter/nf_flow_table_inet.c:32
 nf_hook_entry_hookfn include/linux/netfilter.h:154 [inline]
 nf_hook_slow+0xf4/0x400 net/netfilter/core.c:626
 nf_hook_ingress include/linux/netfilter_netdev.h:34 [inline]
 nf_ingress net/core/dev.c:5440 [inline]

Fixes: 4cd91f7c290f ("netfilter: flowtable: add vlan support")
Reported-by: syzbot+8407d9bb88cd4c6bf61a@syzkaller.appspotmail.com
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_inet.c | 3 +++
 net/netfilter/nf_flow_table_ip.c   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/net/netfilter/nf_flow_table_inet.c b/net/netfilter/nf_flow_table_inet.c
index 88787b45e30d..8b541a080342 100644
--- a/net/netfilter/nf_flow_table_inet.c
+++ b/net/netfilter/nf_flow_table_inet.c
@@ -17,6 +17,9 @@ nf_flow_offload_inet_hook(void *priv, struct sk_buff *skb,
 
 	switch (skb->protocol) {
 	case htons(ETH_P_8021Q):
+		if (!pskb_may_pull(skb, skb_mac_offset(skb) + sizeof(*veth)))
+			return NF_ACCEPT;
+
 		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
 		proto = veth->h_vlan_encapsulated_proto;
 		break;
diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index c2c005234dcd..98edcaa37b38 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -281,6 +281,9 @@ static bool nf_flow_skb_encap_protocol(struct sk_buff *skb, __be16 proto,
 
 	switch (skb->protocol) {
 	case htons(ETH_P_8021Q):
+		if (!pskb_may_pull(skb, skb_mac_offset(skb) + sizeof(*veth)))
+			return false;
+
 		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
 		if (veth->h_vlan_encapsulated_proto == proto) {
 			*offset += VLAN_HLEN;
-- 
2.30.2

Re: [PATCH net 3/3] netfilter: flowtable: validate vlan header

[Pablo Neira Ayuso]

On Thu, Aug 22, 2024 at 08:39:56AM +0200, Eric Dumazet wrote:
> On Thu, Aug 22, 2024 at 2:17 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > Ensure there is sufficient room to access the protocol field of the
> > VLAN header, validate it once before the flowtable lookup.
> >
> > =====================================================
> > BUG: KMSAN: uninit-value in nf_flow_offload_inet_hook+0x45a/0x5f0 net/netfilter/nf_flow_table_inet.c:32
> >  nf_flow_offload_inet_hook+0x45a/0x5f0 net/netfilter/nf_flow_table_inet.c:32
> >  nf_hook_entry_hookfn include/linux/netfilter.h:154 [inline]
> >  nf_hook_slow+0xf4/0x400 net/netfilter/core.c:626
> >  nf_hook_ingress include/linux/netfilter_netdev.h:34 [inline]
> >  nf_ingress net/core/dev.c:5440 [inline]
> >
> > Fixes: 4cd91f7c290f ("netfilter: flowtable: add vlan support")
> > Reported-by: syzbot+8407d9bb88cd4c6bf61a@syzkaller.appspotmail.com
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/nf_flow_table_inet.c | 3 +++
> >  net/netfilter/nf_flow_table_ip.c   | 3 +++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/net/netfilter/nf_flow_table_inet.c b/net/netfilter/nf_flow_table_inet.c
> > index 88787b45e30d..dd9a392052ee 100644
> > --- a/net/netfilter/nf_flow_table_inet.c
> > +++ b/net/netfilter/nf_flow_table_inet.c
> > @@ -17,6 +17,9 @@ nf_flow_offload_inet_hook(void *priv, struct sk_buff *skb,
> >
> >         switch (skb->protocol) {
> >         case htons(ETH_P_8021Q):
> > +               if (!pskb_may_pull(skb, VLAN_HLEN))
> > +                       return NF_ACCEPT;
> > +
> >                 veth = (struct vlan_ethhdr *)skb_mac_header(skb);
> 
> Is skb_mac_header(skb) always pointing at skb->data - 14 at this stage ?
> 
> Otherwise, using
> 
>   if (!pskb_may_pull(skb, skb_mac_offset(skb) + sizeof(*veth))  would be safer.

That looks consistent. Done and I just sent a new PR. Thanks.

Re: [PATCH net 1/3] netfilter: nft_counter: Disable BH in nft_counter_offload_stats().

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Pablo Neira Ayuso <pablo@netfilter.org>:

On Thu, 22 Aug 2024 12:18:40 +0200 you wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> The sequence counter nft_counter_seq is a per-CPU counter. There is no
> lock associated with it. nft_counter_do_eval() is using the same counter
> and disables BH which suggest that it can be invoked from a softirq.
> This in turn means that nft_counter_offload_stats(), which disables only
> preemption, can be interrupted by nft_counter_do_eval() leading to two
> writer for one seqcount_t.
> This can lead to loosing stats or reading statistics while they are
> updated.
> 
> [...]

Here is the summary with links:
  - [net,1/3] netfilter: nft_counter: Disable BH in nft_counter_offload_stats().
    https://git.kernel.org/netdev/net/c/1eacdd71b343
  - [net,2/3] netfilter: nft_counter: Synchronize nft_counter_reset() against reader.
    https://git.kernel.org/netdev/net/c/a0b39e2dc701
  - [net,3/3] netfilter: flowtable: validate vlan header
    https://git.kernel.org/netdev/net/c/6ea14ccb60c8

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