netdev - Re: [PATCH net 1/3] net/sched: act_ipt: add sanity checks on table name and hook locations

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]

Date: Thu, 8 Jun 2023 12:44:25 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Florian Westphal <fw@...len.de>
Cc: netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
	jiri@...nulli.us
Subject: Re: [PATCH net 1/3] net/sched: act_ipt: add sanity checks on table
 name and hook locations

On Wed, Jun 07, 2023 at 04:59:52PM +0200, Florian Westphal wrote:
> Looks like "tc" hard-codes "mangle" as the only supported table
> name, but on kernel side there are no checks.
> 
> This is wrong.  Not all xtables targets are safe to call from tc.
> E.g. "nat" targets assume skb has a conntrack object assigned to it.
> Normally those get called from netfilter nat core which consults the
> nat table to obtain the address mapping.
> 
> "tc" userspace either sets PRE or POSTROUTING as hook number, but there
> is no validation of this on kernel side, so update netlink policy to
> reject bogus numbers.  Some targets may assume skb_dst is set for
> input/forward hooks, so prevent those from being used.
> 
> act_ipt uses the hook number in two places:
> 1. the state hook number, this is fine as-is
> 2. to set par.hook_mask
> 
> The latter is a bit mask, so update the assignment to make
> xt_check_target() to the right thing.
> 
> Followup patch adds required checks for the skb/packet headers before
> calling the targets evaluation function.
> 
> Signed-off-by: Florian Westphal <fw@...len.de>

Hi Florian,

I think that patches for 'net' usually come with a fixes tag.
Likewise for the other patches in this series.

That aside, this looks good to me.

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