linux-kernel - Re: [PATCH v2] netfilter: Make xt_cgroup independent from net

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: <Z9_SSuPu2TXeN2TD@calendula>
Date: Sun, 23 Mar 2025 10:20:10 +0100
From: Pablo Neira Ayuso <pablo@...filter.org>
To: Michal Koutný <mkoutny@...e.com>
Cc: netfilter-devel@...r.kernel.org, coreteam@...filter.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jozsef Kadlecsik <kadlec@...filter.org>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, cgroups@...r.kernel.org,
	Jan Engelhardt <ej@...i.de>, Florian Westphal <fw@...len.de>
Subject: Re: [PATCH v2] netfilter: Make xt_cgroup independent from net_cls

Hi Michal,

I have one question.

On Wed, Mar 05, 2025 at 06:09:35PM +0100, Michal Koutný wrote:
> diff --git a/net/netfilter/xt_cgroup.c b/net/netfilter/xt_cgroup.c
> index c0f5e9a4f3c65..c3055e74aa0ea 100644
> --- a/net/netfilter/xt_cgroup.c
> +++ b/net/netfilter/xt_cgroup.c
> @@ -23,6 +23,13 @@ MODULE_DESCRIPTION("Xtables: process control group matching");
>  MODULE_ALIAS("ipt_cgroup");
>  MODULE_ALIAS("ip6t_cgroup");
>  
> +#define NET_CLS_CLASSID_INVALID_MSG "xt_cgroup: classid invalid without net_cls cgroups\n"
> +
> +static bool possible_classid(u32 classid)
> +{
> +	return IS_ENABLED(CONFIG_CGROUP_NET_CLASSID) || classid == 0;
> +}
> +
>  static int cgroup_mt_check_v0(const struct xt_mtchk_param *par)
>  {
>  	struct xt_cgroup_info_v0 *info = par->matchinfo;
> @@ -30,6 +37,11 @@ static int cgroup_mt_check_v0(const struct xt_mtchk_param *par)
>  	if (info->invert & ~1)
>  		return -EINVAL;
>  
> +	if (!possible_classid(info->id)) {

why classid != 0 is accepted for cgroup_mt_check_v0()?

cgroup_mt_check_v0 represents revision 0 of this match, and this match
only supports for clsid (groupsv1).

History of revisions of cgroupsv2:

- cgroup_mt_check_v0 added to match on clsid (initial version of this match)
- cgroup_mt_check_v1 is added to support cgroupsv2 matching 
- cgroup_mt_check_v2 is added to make cgroupsv2 matching more flexible

I mean, if !IS_ENABLED(CONFIG_CGROUP_NET_CLASSID) then xt_cgroup
should fail for cgroup_mt_check_v0.

But a more general question: why this check for classid == 0 in
cgroup_mt_check_v1 and cgroup_mt_check_v2?

> +		pr_info(NET_CLS_CLASSID_INVALID_MSG);
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -51,6 +63,11 @@ static int cgroup_mt_check_v1(const struct xt_mtchk_param *par)
>  		return -EINVAL;
>  	}
>  
> +	if (!possible_classid(info->classid)) {
> +		pr_info(NET_CLS_CLASSID_INVALID_MSG);
> +		return -EINVAL;
> +	}
> +
>  	info->priv = NULL;
>  	if (info->has_path) {
>  		cgrp = cgroup_get_from_path(info->path);
> @@ -83,6 +100,11 @@ static int cgroup_mt_check_v2(const struct xt_mtchk_param *par)
>  		return -EINVAL;
>  	}
>  
> +	if (info->has_classid && !possible_classid(info->classid)) {
> +		pr_info(NET_CLS_CLASSID_INVALID_MSG);
> +		return -EINVAL;
> +	}
> +
>  	info->priv = NULL;
>  	if (info->has_path) {
>  		cgrp = cgroup_get_from_path(info->path);
> 
> base-commit: dd83757f6e686a2188997cb58b5975f744bb7786
> -- 
> 2.48.1
>