All of lore.kernel.org
 help / color / mirror / Atom feed
From: Simon Horman <simon.horman@corigine.com>
To: Florian Westphal <fw@strlen.de>
Cc: netdev@vger.kernel.org, jhs@mojatatu.com,
	xiyou.wangcong@gmail.com, jiri@resnulli.us
Subject: Re: [PATCH net 3/3] net/sched: act_ipt: zero skb->cb before calling target
Date: Thu, 8 Jun 2023 12:45:38 +0200	[thread overview]
Message-ID: <ZIGxUtaYoRl50Ldv@corigine.com> (raw)
In-Reply-To: <20230607145954.19324-4-fw@strlen.de>

On Wed, Jun 07, 2023 at 04:59:54PM +0200, Florian Westphal 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.
> 
> 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.
> 
> 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 explcit list of allowed

nit: explcit -> explicit

> target extensions, that only return DROP or ACCEPT verdicts, but this
> seems to be error prone.
> 
> Existing selftest only validates xt_LOG and act_ipt is restricted
> to ipv4 so I don't think this action is used widely.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Reviewed-by: Simon Horman <simon.horman@corigine.com>


      reply	other threads:[~2023-06-08 10:45 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-07 14:59 [PATCH net 0/3] net/sched: act_ipt bug fixes Florian Westphal
2023-06-07 14:59 ` [PATCH net 1/3] net/sched: act_ipt: add sanity checks on table name and hook locations Florian Westphal
2023-06-08 10:44   ` Simon Horman
2023-06-08 13:57     ` Florian Westphal
2023-06-08 16:44       ` Jamal Hadi Salim
2023-06-07 14:59 ` [PATCH net 2/3] net/sched: act_ipt: add sanity checks on skb before calling target Florian Westphal
2023-06-08 10:44   ` Simon Horman
2023-06-07 14:59 ` [PATCH net 3/3] net/sched: act_ipt: zero skb->cb " Florian Westphal
2023-06-08 10:45   ` Simon Horman [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZIGxUtaYoRl50Ldv@corigine.com \
    --to=simon.horman@corigine.com \
    --cc=fw@strlen.de \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=netdev@vger.kernel.org \
    --cc=xiyou.wangcong@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.