| Message ID | 20230608140246.15190-3-fw@strlen.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: act_ipt bug fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 48 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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!
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 >
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...
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,
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);
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(+)