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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAM0EoMkvUzSPH26_U_uv80wGsUE2V28COb-7Bi3fjjOYc9W+zA@mail.gmail.com>
Date: Fri, 9 Jun 2023 07:58:47 -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 2:28 PM Florian Westphal <fw@...len.de> wrote:
>
> Jamal Hadi Salim <jhs@...atatu.com> wrote:
> > 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.
>
> 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