| Message ID | 20230608140246.15190-4-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, 40 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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 >
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.
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
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; }
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(+)