netdev - Re: [PATCH net v2 2/3] net/sched: act_ipt: add sanity checks on skb 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAM0EoMkDHQYdL+0cgOzbEemAOfFXPJSe-z0Bgez=YbyQ56iLoQ@mail.gmail.com>
Date: Thu, 8 Jun 2023 13:08:23 -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 2/3] net/sched: act_ipt: add sanity checks on skb
 before calling target

On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@...len.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@...len.de>
> Reviewed-by: Simon Horman <simon.horman@...igine.com>

Other than the comment from Davide (which makes sense) I would say:
Acked-by: Jamal Hadi Salim <jhs@...atatu.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
>