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: <20230608182831.GE27126@breakpoint.cc>
Date: Thu, 8 Jun 2023 20:28:31 +0200
From: Florian Westphal <fw@...len.de>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Florian Westphal <fw@...len.de>, 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

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...

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.