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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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
>