[PATCH net v2 0/3] net/sched: act_ipt bug fixes

[PATCH net v2 0/3] net/sched: act_ipt bug fixes

[Florian Westphal]

v2: add "Fixes" tags, no other changes, so retaining Simons RvB tag.

While checking if netfilter could be updated to replace selected
instances of NF_DROP with kfree_skb_reason+NF_STOLEN to improve
debugging info via drop monitor I found that act_ipt is incompatible
with such an approach.  Moreover, it lacks multiple sanity checks
to avoid certain code paths that make assumptions that the tc layer
doesn't meet, such as header sanity checks, availability of skb_dst
skb_nfct() and so on.

act_ipt test in the tc selftest still pass with this applied.

I think that we should consider removal of this module, while
this should take care of all problems, its ipv4 only and I don't
think there are any netfilter targets that lack a native tc
equivalent, even when ignoring bpf.

Florian Westphal (3):
  net/sched: act_ipt: add sanity checks on table name and hook locations
  net/sched: act_ipt: add sanity checks on skb before calling target
  net/sched: act_ipt: zero skb->cb before calling target

 net/sched/act_ipt.c | 70 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 7 deletions(-)

-- 
2.40.1

[PATCH net v2 1/3] net/sched: act_ipt: add sanity checks on table name and hook locations

[Florian Westphal]

Looks like "tc" hard-codes "mangle" as the only supported table
name, but on kernel side there are no checks.

This is wrong.  Not all xtables targets are safe to call from tc.
E.g. "nat" targets assume skb has a conntrack object assigned to it.
Normally those get called from netfilter nat core which consults the
nat table to obtain the address mapping.

"tc" userspace either sets PRE or POSTROUTING as hook number, but there
is no validation of this on kernel side, so update netlink policy to
reject bogus numbers.  Some targets may assume skb_dst is set for
input/forward hooks, so prevent those from being used.

act_ipt uses the hook number in two places:
1. the state hook number, this is fine as-is
2. to set par.hook_mask

The latter is a bit mask, so update the assignment to make
xt_check_target() to the right thing.

Followup patch adds required checks for the skb/packet headers before
calling the targets evaluation function.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Westphal <fw@strlen.de>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
 v2: add Fixes tag, diff unchanged.

 net/sched/act_ipt.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 5d96ffebd40f..ea7f151e7dd2 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -48,7 +48,7 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t,
 	par.entryinfo = &e;
 	par.target    = target;
 	par.targinfo  = t->data;
-	par.hook_mask = hook;
+	par.hook_mask = 1 << hook;
 	par.family    = NFPROTO_IPV4;
 
 	ret = xt_check_target(&par, t->u.target_size - sizeof(*t), 0, false);
@@ -85,7 +85,8 @@ static void tcf_ipt_release(struct tc_action *a)
 
 static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
 	[TCA_IPT_TABLE]	= { .type = NLA_STRING, .len = IFNAMSIZ },
-	[TCA_IPT_HOOK]	= { .type = NLA_U32 },
+	[TCA_IPT_HOOK]	= NLA_POLICY_RANGE(NLA_U32, NF_INET_PRE_ROUTING,
+					   NF_INET_NUMHOOKS),
 	[TCA_IPT_INDEX]	= { .type = NLA_U32 },
 	[TCA_IPT_TARG]	= { .len = sizeof(struct xt_entry_target) },
 };
@@ -158,15 +159,27 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
 			return -EEXIST;
 		}
 	}
+
+	err = -EINVAL;
 	hook = nla_get_u32(tb[TCA_IPT_HOOK]);
+	switch (hook) {
+	case NF_INET_PRE_ROUTING:
+		break;
+	case NF_INET_POST_ROUTING:
+		break;
+	default:
+		goto err1;
+	}
+
+	if (tb[TCA_IPT_TABLE]) {
+		/* mangle only for now */
+		if (nla_strcmp(tb[TCA_IPT_TABLE], "mangle"))
+			goto err1;
+	}
 
-	err = -ENOMEM;
-	tname = kmalloc(IFNAMSIZ, GFP_KERNEL);
+	tname = kstrdup("mangle", GFP_KERNEL);
 	if (unlikely(!tname))
 		goto err1;
-	if (tb[TCA_IPT_TABLE] == NULL ||
-	    nla_strscpy(tname, tb[TCA_IPT_TABLE], IFNAMSIZ) >= IFNAMSIZ)
-		strcpy(tname, "mangle");
 
 	t = kmemdup(td, td->u.target_size, GFP_KERNEL);
 	if (unlikely(!t))
-- 
2.40.1

[PATCH net v2 2/3] net/sched: act_ipt: add sanity checks on skb before calling target

[Florian Westphal]

Netfilter targets make assumptions on the skb state, for example
iphdr is supposed to be in the linear area.

This is normally done by IP stack, but in act_ipt case no
such checks are made.

Some targets can even assume that skb_dst will be valid.
Make a minimum effort to check for this:

- Don't call the targets eval function for non-ipv4 skbs.
- Don't call the targets eval function for POSTROUTING
  emulation when the skb has no dst set.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Westphal <fw@strlen.de>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
 v2: add Fixes tag, diff unchanged.

 net/sched/act_ipt.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index ea7f151e7dd2..2f0b39cc4e37 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -230,6 +230,26 @@ static int tcf_xt_init(struct net *net, struct nlattr *nla,
 			      a, &act_xt_ops, tp, flags);
 }
 
+static bool tcf_ipt_act_check(struct sk_buff *skb)
+{
+	const struct iphdr *iph;
+	unsigned int nhoff, len;
+
+	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
+		return false;
+
+	nhoff = skb_network_offset(skb);
+	iph = ip_hdr(skb);
+	if (iph->ihl < 5 || iph->version != 4)
+		return false;
+
+	len = skb_ip_totlen(skb);
+	if (skb->len < nhoff + len || len < (iph->ihl * 4u))
+		return false;
+
+	return pskb_may_pull(skb, iph->ihl * 4u);
+}
+
 TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
 				  const struct tc_action *a,
 				  struct tcf_result *res)
@@ -244,9 +264,22 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
 		.pf	= NFPROTO_IPV4,
 	};
 
+	if (skb->protocol != htons(ETH_P_IP))
+		return TC_ACT_UNSPEC;
+
 	if (skb_unclone(skb, GFP_ATOMIC))
 		return TC_ACT_UNSPEC;
 
+	if (!tcf_ipt_act_check(skb))
+		return TC_ACT_UNSPEC;
+
+	if (state.hook == NF_INET_POST_ROUTING) {
+		if (!skb_dst(skb))
+			return TC_ACT_UNSPEC;
+
+		state.out = skb->dev;
+	}
+
 	spin_lock(&ipt->tcf_lock);
 
 	tcf_lastuse_update(&ipt->tcf_tm);
-- 
2.40.1

[PATCH net v2 3/3] net/sched: act_ipt: zero skb->cb before calling target

[Florian Westphal]

xtables relies on skb being owned by ip stack, i.e. with ipv4
check in place skb->cb is supposed to be IPCB.

I don't see an immediate problem (REJECT target cannot be used anymore
now that PRE/POSTROUTING hook validation has been fixed), but better be
safe than sorry.

A much better patch would be to either mark act_ipt as
"depends on BROKEN" or remove it altogether. I plan to do this
for -next in the near future.

This tc extension is broken in the sense that tc lacks an
equivalent of NF_STOLEN verdict.

With NF_STOLEN, target function takes complete ownership of skb, caller
cannot dereference it anymore.

ACT_STOLEN cannot be used for this: it has a different meaning, caller
is allowed to dereference the skb.

At this time NF_STOLEN won't be returned by any targets as far as I can
see, but this may change in the future.

It might be possible to work around this via list of allowed
target extensions known to only return DROP or ACCEPT verdicts, but this
is error prone/fragile.

Existing selftest only validates xt_LOG and act_ipt is restricted
to ipv4 so I don't think this action is used widely.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Florian Westphal <fw@strlen.de>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
 v2: add Fixes tag, fix typo in commit message, diff unchanged.

 net/sched/act_ipt.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 2f0b39cc4e37..ec04bcfa0f4b 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -21,6 +21,7 @@
 #include <linux/tc_act/tc_ipt.h>
 #include <net/tc_act/tc_ipt.h>
 #include <net/tc_wrapper.h>
+#include <net/ip.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 
@@ -254,6 +255,7 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
 				  const struct tc_action *a,
 				  struct tcf_result *res)
 {
+	char saved_cb[sizeof_field(struct sk_buff, cb)];
 	int ret = 0, result = 0;
 	struct tcf_ipt *ipt = to_ipt(a);
 	struct xt_action_param par;
@@ -280,6 +282,8 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
 		state.out = skb->dev;
 	}
 
+	memcpy(saved_cb, skb->cb, sizeof(saved_cb));
+
 	spin_lock(&ipt->tcf_lock);
 
 	tcf_lastuse_update(&ipt->tcf_tm);
@@ -292,6 +296,9 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
 	par.state    = &state;
 	par.target   = ipt->tcfi_t->u.kernel.target;
 	par.targinfo = ipt->tcfi_t->data;
+
+	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
+
 	ret = par.target->target(skb, &par);
 
 	switch (ret) {
@@ -312,6 +319,9 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
 		break;
 	}
 	spin_unlock(&ipt->tcf_lock);
+
+	memcpy(skb->cb, saved_cb, sizeof(skb->cb));
+
 	return result;
 
 }
-- 
2.40.1

Re: [PATCH net v2 2/3] net/sched: act_ipt: add sanity checks on skb before calling target

[Davide Caratti]

hello Florian,

On Thu, Jun 8, 2023 at 4:04 PM Florian Westphal <fw@strlen.de> wrote:
>
> Netfilter targets make assumptions on the skb state, for example
> iphdr is supposed to be in the linear area.
>
[...]

> @@ -244,9 +264,22 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
>                 .pf     = NFPROTO_IPV4,
>         };
>
> +       if (skb->protocol != htons(ETH_P_IP))
> +               return TC_ACT_UNSPEC;
> +

maybe this can be converted to skb_protocol(skb, ...)  so that it's
clear how VLAN packets are treated ?
thanks!
-- 
davide

Re: [PATCH net v2 0/3] net/sched: act_ipt bug fixes

[Jamal Hadi Salim]

On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@strlen.de> wrote:
>
> v2: add "Fixes" tags, no other changes, so retaining Simons RvB tag.
>
> While checking if netfilter could be updated to replace selected
> instances of NF_DROP with kfree_skb_reason+NF_STOLEN to improve
> debugging info via drop monitor I found that act_ipt is incompatible
> with such an approach.  Moreover, it lacks multiple sanity checks
> to avoid certain code paths that make assumptions that the tc layer
> doesn't meet, such as header sanity checks, availability of skb_dst
> skb_nfct() and so on.
>
> act_ipt test in the tc selftest still pass with this applied.
>
> I think that we should consider removal of this module, while
> this should take care of all problems, its ipv4 only and I don't
> think there are any netfilter targets that lack a native tc
> equivalent, even when ignoring bpf.
>

I am all for removing it - but i am worried there are users based on
past interactions. Will try to ping some users and see if they
actually were using it. I will send a patch to retire it if it looks
safe.
Unfortunately ipt suffered because we couldnt coordinate between the
netfilter and tc subsystem. In the old days so calling/callee bugs
fixed in the netfilter world find their way into ipt. At some point
that stopped. Also the user space interfacing suffered from similar
issues.

cheers,
jamal

> Florian Westphal (3):
>   net/sched: act_ipt: add sanity checks on table name and hook locations
>   net/sched: act_ipt: add sanity checks on skb before calling target
>   net/sched: act_ipt: zero skb->cb before calling target
>
>  net/sched/act_ipt.c | 70 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 7 deletions(-)
>
> --
> 2.40.1
>

Re: [PATCH net v2 1/3] net/sched: act_ipt: add sanity checks on table name and hook locations

[Jamal Hadi Salim]

On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@strlen.de> wrote:
>
> Looks like "tc" hard-codes "mangle" as the only supported table
> name, but on kernel side there are no checks.
>
> This is wrong.  Not all xtables targets are safe to call from tc.
> E.g. "nat" targets assume skb has a conntrack object assigned to it.
> Normally those get called from netfilter nat core which consults the
> nat table to obtain the address mapping.
>
> "tc" userspace either sets PRE or POSTROUTING as hook number, but there
> is no validation of this on kernel side, so update netlink policy to
> reject bogus numbers.  Some targets may assume skb_dst is set for
> input/forward hooks, so prevent those from being used.
>
> act_ipt uses the hook number in two places:
> 1. the state hook number, this is fine as-is
> 2. to set par.hook_mask
>
> The latter is a bit mask, so update the assignment to make
> xt_check_target() to the right thing.
>
> Followup patch adds required checks for the skb/packet headers before
> calling the targets evaluation function.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

> ---
>  v2: add Fixes tag, diff unchanged.
>
>  net/sched/act_ipt.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
> index 5d96ffebd40f..ea7f151e7dd2 100644
> --- a/net/sched/act_ipt.c
> +++ b/net/sched/act_ipt.c
> @@ -48,7 +48,7 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t,
>         par.entryinfo = &e;
>         par.target    = target;
>         par.targinfo  = t->data;
> -       par.hook_mask = hook;
> +       par.hook_mask = 1 << hook;
>         par.family    = NFPROTO_IPV4;
>
>         ret = xt_check_target(&par, t->u.target_size - sizeof(*t), 0, false);
> @@ -85,7 +85,8 @@ static void tcf_ipt_release(struct tc_action *a)
>
>  static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
>         [TCA_IPT_TABLE] = { .type = NLA_STRING, .len = IFNAMSIZ },
> -       [TCA_IPT_HOOK]  = { .type = NLA_U32 },
> +       [TCA_IPT_HOOK]  = NLA_POLICY_RANGE(NLA_U32, NF_INET_PRE_ROUTING,
> +                                          NF_INET_NUMHOOKS),
>         [TCA_IPT_INDEX] = { .type = NLA_U32 },
>         [TCA_IPT_TARG]  = { .len = sizeof(struct xt_entry_target) },
>  };
> @@ -158,15 +159,27 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
>                         return -EEXIST;
>                 }
>         }
> +
> +       err = -EINVAL;
>         hook = nla_get_u32(tb[TCA_IPT_HOOK]);
> +       switch (hook) {
> +       case NF_INET_PRE_ROUTING:
> +               break;
> +       case NF_INET_POST_ROUTING:
> +               break;
> +       default:
> +               goto err1;
> +       }
> +
> +       if (tb[TCA_IPT_TABLE]) {
> +               /* mangle only for now */
> +               if (nla_strcmp(tb[TCA_IPT_TABLE], "mangle"))
> +                       goto err1;
> +       }
>
> -       err = -ENOMEM;
> -       tname = kmalloc(IFNAMSIZ, GFP_KERNEL);
> +       tname = kstrdup("mangle", GFP_KERNEL);
>         if (unlikely(!tname))
>                 goto err1;
> -       if (tb[TCA_IPT_TABLE] == NULL ||
> -           nla_strscpy(tname, tb[TCA_IPT_TABLE], IFNAMSIZ) >= IFNAMSIZ)
> -               strcpy(tname, "mangle");
>
>         t = kmemdup(td, td->u.target_size, GFP_KERNEL);
>         if (unlikely(!t))
> --
> 2.40.1
>

Re: [PATCH net v2 3/3] net/sched: act_ipt: zero skb->cb before calling target

[Jamal Hadi Salim]

On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@strlen.de> wrote:
>
> xtables relies on skb being owned by ip stack, i.e. with ipv4
> check in place skb->cb is supposed to be IPCB.
>
> I don't see an immediate problem (REJECT target cannot be used anymore
> now that PRE/POSTROUTING hook validation has been fixed), but better be
> safe than sorry.
>
> A much better patch would be to either mark act_ipt as
> "depends on BROKEN" or remove it altogether. I plan to do this
> for -next in the near future.

Let me handle this part please.

> This tc extension is broken in the sense that tc lacks an
> equivalent of NF_STOLEN verdict.
> With NF_STOLEN, target function takes complete ownership of skb, caller
> cannot dereference it anymore.
>
> ACT_STOLEN cannot be used for this: it has a different meaning, caller
> is allowed to dereference the skb.
>

ACT_STOLEN requires that the target clones the packet and the caller
to free the skb.

> At this time NF_STOLEN won't be returned by any targets as far as I can
> see, but this may change in the future.
>
> It might be possible to work around this via list of allowed
> target extensions known to only return DROP or ACCEPT verdicts, but this
> is error prone/fragile.

I didnt quiet follow why ACT_STOLEN if this action frees the packet
and returns NF_STOLEN

> Existing selftest only validates xt_LOG and act_ipt is restricted
> to ipv4 so I don't think this action is used widely.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>

Other than that:
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

And thank you for doing this Florian.

cheers,
jamal
> ---
>  v2: add Fixes tag, fix typo in commit message, diff unchanged.
>
>  net/sched/act_ipt.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
> index 2f0b39cc4e37..ec04bcfa0f4b 100644
> --- a/net/sched/act_ipt.c
> +++ b/net/sched/act_ipt.c
> @@ -21,6 +21,7 @@
>  #include <linux/tc_act/tc_ipt.h>
>  #include <net/tc_act/tc_ipt.h>
>  #include <net/tc_wrapper.h>
> +#include <net/ip.h>
>
>  #include <linux/netfilter_ipv4/ip_tables.h>
>
> @@ -254,6 +255,7 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
>                                   const struct tc_action *a,
>                                   struct tcf_result *res)
>  {
> +       char saved_cb[sizeof_field(struct sk_buff, cb)];
>         int ret = 0, result = 0;
>         struct tcf_ipt *ipt = to_ipt(a);
>         struct xt_action_param par;
> @@ -280,6 +282,8 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
>                 state.out = skb->dev;
>         }
>
> +       memcpy(saved_cb, skb->cb, sizeof(saved_cb));
> +
>         spin_lock(&ipt->tcf_lock);
>
>         tcf_lastuse_update(&ipt->tcf_tm);
> @@ -292,6 +296,9 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
>         par.state    = &state;
>         par.target   = ipt->tcfi_t->u.kernel.target;
>         par.targinfo = ipt->tcfi_t->data;
> +
> +       memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
> +
>         ret = par.target->target(skb, &par);
>
>         switch (ret) {
> @@ -312,6 +319,9 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
>                 break;
>         }
>         spin_unlock(&ipt->tcf_lock);
> +
> +       memcpy(skb->cb, saved_cb, sizeof(skb->cb));
> +
>         return result;
>
>  }
> --
> 2.40.1
>

Re: [PATCH net v2 2/3] net/sched: act_ipt: add sanity checks on skb before calling target

[Jamal Hadi Salim]

On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@strlen.de> wrote:
>
> Netfilter targets make assumptions on the skb state, for example
> iphdr is supposed to be in the linear area.
>
> This is normally done by IP stack, but in act_ipt case no
> such checks are made.
>
> Some targets can even assume that skb_dst will be valid.
> Make a minimum effort to check for this:
>
> - Don't call the targets eval function for non-ipv4 skbs.
> - Don't call the targets eval function for POSTROUTING
>   emulation when the skb has no dst set.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>

Other than the comment from Davide (which makes sense) I would say:
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal


> ---
>  v2: add Fixes tag, diff unchanged.
>
>  net/sched/act_ipt.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
> index ea7f151e7dd2..2f0b39cc4e37 100644
> --- a/net/sched/act_ipt.c
> +++ b/net/sched/act_ipt.c
> @@ -230,6 +230,26 @@ static int tcf_xt_init(struct net *net, struct nlattr *nla,
>                               a, &act_xt_ops, tp, flags);
>  }
>
> +static bool tcf_ipt_act_check(struct sk_buff *skb)
> +{
> +       const struct iphdr *iph;
> +       unsigned int nhoff, len;
> +
> +       if (!pskb_may_pull(skb, sizeof(struct iphdr)))
> +               return false;
> +
> +       nhoff = skb_network_offset(skb);
> +       iph = ip_hdr(skb);
> +       if (iph->ihl < 5 || iph->version != 4)
> +               return false;
> +
> +       len = skb_ip_totlen(skb);
> +       if (skb->len < nhoff + len || len < (iph->ihl * 4u))
> +               return false;
> +
> +       return pskb_may_pull(skb, iph->ihl * 4u);
> +}
> +
>  TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
>                                   const struct tc_action *a,
>                                   struct tcf_result *res)
> @@ -244,9 +264,22 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
>                 .pf     = NFPROTO_IPV4,
>         };
>
> +       if (skb->protocol != htons(ETH_P_IP))
> +               return TC_ACT_UNSPEC;
> +
>         if (skb_unclone(skb, GFP_ATOMIC))
>                 return TC_ACT_UNSPEC;
>
> +       if (!tcf_ipt_act_check(skb))
> +               return TC_ACT_UNSPEC;
> +
> +       if (state.hook == NF_INET_POST_ROUTING) {
> +               if (!skb_dst(skb))
> +                       return TC_ACT_UNSPEC;
> +
> +               state.out = skb->dev;
> +       }
> +
>         spin_lock(&ipt->tcf_lock);
>
>         tcf_lastuse_update(&ipt->tcf_tm);
> --
> 2.40.1
>

Re: [PATCH net v2 3/3] net/sched: act_ipt: zero skb->cb before calling target

[Florian Westphal]

Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@strlen.de> wrote:
> >
> > xtables relies on skb being owned by ip stack, i.e. with ipv4
> > check in place skb->cb is supposed to be IPCB.
> >
> > I don't see an immediate problem (REJECT target cannot be used anymore
> > now that PRE/POSTROUTING hook validation has been fixed), but better be
> > safe than sorry.
> >
> > A much better patch would be to either mark act_ipt as
> > "depends on BROKEN" or remove it altogether. I plan to do this
> > for -next in the near future.
> 
> Let me handle this part please.

Sure, no problem.

> > This tc extension is broken in the sense that tc lacks an
> > equivalent of NF_STOLEN verdict.
> > With NF_STOLEN, target function takes complete ownership of skb, caller
> > cannot dereference it anymore.
> >
> > ACT_STOLEN cannot be used for this: it has a different meaning, caller
> > is allowed to dereference the skb.
> >
> 
> ACT_STOLEN requires that the target clones the packet and the caller
> to free the skb.

Makes sense, but if NF_STOLEN gets returned the skb is already released,
so we can't touch it anymore.

> > At this time NF_STOLEN won't be returned by any targets as far as I can
> > see, but this may change in the future.
> >
> > It might be possible to work around this via list of allowed
> > target extensions known to only return DROP or ACCEPT verdicts, but this
> > is error prone/fragile.
> 
> I didnt quiet follow why ACT_STOLEN if this action frees the packet
> and returns NF_STOLEN

We could emulate NF_STOLEN via ACT_STOLEN, yes, but we'd have to
skb_clone() unconditionally for every skb before calling the target
eval function...

Other alternatives I can think of:

- keep a list of "known safe" targets,
- annotate all accept-or-drop targets as "safe for act_ipt"
- make the skb shared before calling target function
- ensure that targets will never ever return NF_STOLEN

I dont really like any of these options :-)

At this time, targets return one of accept/drop/queue.

NF_QEUEUE will log an error and treats it like NF_ACCEPT,
so we are good at this time.

Re: [PATCH net v2 2/3] net/sched: act_ipt: add sanity checks on skb before calling target

[Florian Westphal]

Davide Caratti <dcaratti@redhat.com> wrote:
> hello Florian,
> 
> On Thu, Jun 8, 2023 at 4:04 PM Florian Westphal <fw@strlen.de> wrote:
> >
> > Netfilter targets make assumptions on the skb state, for example
> > iphdr is supposed to be in the linear area.
> >
> [...]
> 
> > @@ -244,9 +264,22 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
> >                 .pf     = NFPROTO_IPV4,
> >         };
> >
> > +       if (skb->protocol != htons(ETH_P_IP))
> > +               return TC_ACT_UNSPEC;
> > +
> 
> maybe this can be converted to skb_protocol(skb, ...)  so that it's
> clear how VLAN packets are treated ?

Not sure how to handle this.

act_ipt claims NFPROTO_IPV4; for iptables/nftables one has to use
the interface name ("-i vlan0") to match on the vlan interface.

I don't really want to add code that pulls/pops the vlan headers
in act_ipt...

Re: [PATCH net v2 0/3] net/sched: act_ipt bug fixes

[Florian Westphal]

Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@strlen.de> wrote:
> > I think that we should consider removal of this module, while
> > this should take care of all problems, its ipv4 only and I don't
> > think there are any netfilter targets that lack a native tc
> > equivalent, even when ignoring bpf.
> >
> 
> I am all for removing it - but i am worried there are users based on
> past interactions. Will try to ping some users and see if they
> actually were using it.

Thanks Jamal.  I'd also be interested in what xt module(s) are used,
if any.

> I will send a patch to retire it if it looks
> safe.

Thanks.

Re: [PATCH net v2 3/3] net/sched: act_ipt: zero skb->cb before calling target

[Jamal Hadi Salim]

On Thu, Jun 8, 2023 at 2:28 PM Florian Westphal <fw@strlen.de> wrote:
>
> Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@strlen.de> wrote:
> > >
> > > xtables relies on skb being owned by ip stack, i.e. with ipv4
> > > check in place skb->cb is supposed to be IPCB.
> > >
> > > I don't see an immediate problem (REJECT target cannot be used anymore
> > > now that PRE/POSTROUTING hook validation has been fixed), but better be
> > > safe than sorry.
> > >
> > > A much better patch would be to either mark act_ipt as
> > > "depends on BROKEN" or remove it altogether. I plan to do this
> > > for -next in the near future.
> >
> > Let me handle this part please.
>
> Sure, no problem.
>
> > > This tc extension is broken in the sense that tc lacks an
> > > equivalent of NF_STOLEN verdict.
> > > With NF_STOLEN, target function takes complete ownership of skb, caller
> > > cannot dereference it anymore.
> > >
> > > ACT_STOLEN cannot be used for this: it has a different meaning, caller
> > > is allowed to dereference the skb.
> > >
> >
> > ACT_STOLEN requires that the target clones the packet and the caller
> > to free the skb.
>
> Makes sense, but if NF_STOLEN gets returned the skb is already released,
> so we can't touch it anymore.
>
> > > At this time NF_STOLEN won't be returned by any targets as far as I can
> > > see, but this may change in the future.
> > >
> > > It might be possible to work around this via list of allowed
> > > target extensions known to only return DROP or ACCEPT verdicts, but this
> > > is error prone/fragile.
> >
> > I didnt quiet follow why ACT_STOLEN if this action frees the packet
> > and returns NF_STOLEN
>
> We could emulate NF_STOLEN via ACT_STOLEN, yes, but we'd have to
> skb_clone() unconditionally for every skb before calling the target
> eval function...
>

True.

> Other alternatives I can think of:
>
> - keep a list of "known safe" targets,
> - annotate all accept-or-drop targets as "safe for act_ipt"
> - make the skb shared before calling target function
> - ensure that targets will never ever return NF_STOLEN
>
> I dont really like any of these options :-)
>
> At this time, targets return one of accept/drop/queue.
>
> NF_QEUEUE will log an error and treats it like NF_ACCEPT,
> so we are good at this time.

Since we are shooting to remove this i think it is sufficient.

cheers,
jamal

Re: [PATCH net v2 2/3] net/sched: act_ipt: add sanity checks on skb before calling target

[Davide Caratti]

On Thu, Jun 8, 2023 at 8:34 PM Florian Westphal <fw@strlen.de> wrote:
>
> Davide Caratti <dcaratti@redhat.com> wrote:
> > hello Florian,
> >
> > On Thu, Jun 8, 2023 at 4:04 PM Florian Westphal <fw@strlen.de> wrote:
> > >
> > > @@ -244,9 +264,22 @@ TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb,
> > >                 .pf     = NFPROTO_IPV4,
> > >         };
> > >
> > > +       if (skb->protocol != htons(ETH_P_IP))
> > > +               return TC_ACT_UNSPEC;
> > > +
> >
> > maybe this can be converted to skb_protocol(skb, ...)  so that it's
> > clear how VLAN packets are treated ?
>
> Not sure how to handle this.
>
> act_ipt claims NFPROTO_IPV4; for iptables/nftables one has to use
> the interface name ("-i vlan0") to match on the vlan interface.
>
> I don't really want to add code that pulls/pops the vlan headers
> in act_ipt...

then probably we can just call

skb_protocol(skb, false)

and check if it's equal to htons(ETH_P_IP):
In this case it will use skb->protocol or skb->vlan_proto (if the tag
is "accelerated") - no recursive QinQ lookup. WDYT?
thanks,
-- 
davide