Merge branch 'net-sched-act_ipt-bug-fixes'

Florian Westphal says:

====================
net/sched: act_ipt bug fixes

v3: prefer skb_header() helper in patch 2. No other changes.
I've retained Acks and RvB-Tags of v2.

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.
====================

Link: https://lore.kernel.org/r/20230627123813.3036-1-fw@strlen.de
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Paolo Abeni 3 years ago 3c4bb45a 6feb37b3

+64 -8

1 changed file

expand all

net

sched

act_ipt.c

+64 -8

net/sched/act_ipt.c

··· 21 21 #include <linux/tc_act/tc_ipt.h> 22 22 #include <net/tc_act/tc_ipt.h> 23 23 #include <net/tc_wrapper.h> 24 + #include <net/ip.h> 24 25 25 26 #include <linux/netfilter_ipv4/ip_tables.h> 26 27 ··· 49 48 par.entryinfo = &e; 50 49 par.target = target; 51 50 par.targinfo = t->data; 52 - par.hook_mask = hook; 51 + par.hook_mask = 1 << hook; 53 52 par.family = NFPROTO_IPV4; 54 53 55 54 ret = xt_check_target(&par, t->u.target_size - sizeof(*t), 0, false); ··· 86 85 87 86 static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = { 88 87 [TCA_IPT_TABLE] = { .type = NLA_STRING, .len = IFNAMSIZ }, 89 - [TCA_IPT_HOOK] = { .type = NLA_U32 }, 88 + [TCA_IPT_HOOK] = NLA_POLICY_RANGE(NLA_U32, NF_INET_PRE_ROUTING, 89 + NF_INET_NUMHOOKS), 90 90 [TCA_IPT_INDEX] = { .type = NLA_U32 }, 91 91 [TCA_IPT_TARG] = { .len = sizeof(struct xt_entry_target) }, 92 92 }; ··· 160 158 return -EEXIST; 161 159 } 162 160 } 163 - hook = nla_get_u32(tb[TCA_IPT_HOOK]); 164 161 165 - err = -ENOMEM; 166 - tname = kmalloc(IFNAMSIZ, GFP_KERNEL); 162 + err = -EINVAL; 163 + hook = nla_get_u32(tb[TCA_IPT_HOOK]); 164 + switch (hook) { 165 + case NF_INET_PRE_ROUTING: 166 + break; 167 + case NF_INET_POST_ROUTING: 168 + break; 169 + default: 170 + goto err1; 171 + } 172 + 173 + if (tb[TCA_IPT_TABLE]) { 174 + /* mangle only for now */ 175 + if (nla_strcmp(tb[TCA_IPT_TABLE], "mangle")) 176 + goto err1; 177 + } 178 + 179 + tname = kstrdup("mangle", GFP_KERNEL); 167 180 if (unlikely(!tname)) 168 181 goto err1; 169 - if (tb[TCA_IPT_TABLE] == NULL || 170 - nla_strscpy(tname, tb[TCA_IPT_TABLE], IFNAMSIZ) >= IFNAMSIZ) 171 - strcpy(tname, "mangle"); 172 182 173 183 t = kmemdup(td, td->u.target_size, GFP_KERNEL); 174 184 if (unlikely(!t)) ··· 231 217 a, &act_xt_ops, tp, flags); 232 218 } 233 219 220 + static bool tcf_ipt_act_check(struct sk_buff *skb) 221 + { 222 + const struct iphdr *iph; 223 + unsigned int nhoff, len; 224 + 225 + if (!pskb_may_pull(skb, sizeof(struct iphdr))) 226 + return false; 227 + 228 + nhoff = skb_network_offset(skb); 229 + iph = ip_hdr(skb); 230 + if (iph->ihl < 5 || iph->version != 4) 231 + return false; 232 + 233 + len = skb_ip_totlen(skb); 234 + if (skb->len < nhoff + len || len < (iph->ihl * 4u)) 235 + return false; 236 + 237 + return pskb_may_pull(skb, iph->ihl * 4u); 238 + } 239 + 234 240 TC_INDIRECT_SCOPE int tcf_ipt_act(struct sk_buff *skb, 235 241 const struct tc_action *a, 236 242 struct tcf_result *res) 237 243 { 244 + char saved_cb[sizeof_field(struct sk_buff, cb)]; 238 245 int ret = 0, result = 0; 239 246 struct tcf_ipt *ipt = to_ipt(a); 240 247 struct xt_action_param par; ··· 266 231 .pf = NFPROTO_IPV4, 267 232 }; 268 233 234 + if (skb_protocol(skb, false) != htons(ETH_P_IP)) 235 + return TC_ACT_UNSPEC; 236 + 269 237 if (skb_unclone(skb, GFP_ATOMIC)) 270 238 return TC_ACT_UNSPEC; 239 + 240 + if (!tcf_ipt_act_check(skb)) 241 + return TC_ACT_UNSPEC; 242 + 243 + if (state.hook == NF_INET_POST_ROUTING) { 244 + if (!skb_dst(skb)) 245 + return TC_ACT_UNSPEC; 246 + 247 + state.out = skb->dev; 248 + } 249 + 250 + memcpy(saved_cb, skb->cb, sizeof(saved_cb)); 271 251 272 252 spin_lock(&ipt->tcf_lock); 273 253 ··· 296 246 par.state = &state; 297 247 par.target = ipt->tcfi_t->u.kernel.target; 298 248 par.targinfo = ipt->tcfi_t->data; 249 + 250 + memset(IPCB(skb), 0, sizeof(struct inet_skb_parm)); 251 + 299 252 ret = par.target->target(skb, &par); 300 253 301 254 switch (ret) { ··· 319 266 break; 320 267 } 321 268 spin_unlock(&ipt->tcf_lock); 269 + 270 + memcpy(skb->cb, saved_cb, sizeof(skb->cb)); 271 + 322 272 return result; 323 273 324 274 }

Configure Feed

Configure Feed