[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM0EoMkMxOwxNVANaYjd6GBFOkkhkNz=n9xyTnLR6=OmB9nVAw@mail.gmail.com>
Date: Thu, 8 Jun 2023 13:05:48 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Florian Westphal <fw@...len.de>
Cc: netdev@...r.kernel.org, kuba@...nel.org, edumazet@...gle.com,
davem@...emloft.net, pabeni@...hat.com, xiyou.wangcong@...il.com,
jiri@...nulli.us, Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH net v2 3/3] net/sched: act_ipt: zero skb->cb before
calling target
On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@...len.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@...len.de>
> Reviewed-by: Simon Horman <simon.horman@...igine.com>
Other than that:
Acked-by: Jamal Hadi Salim <jhs@...atatu.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
>
Powered by blists - more mailing lists