netdev - RE: [PATCH v2 2/2] Make num

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] [day] [month] [year] [list]

Date: Fri, 29 Sep 2023 08:10:00 +0000
From: David Laight <David.Laight@...LAB.COM>
To: "'joao@...rdrivepizza.com'" <joao@...rdrivepizza.com>,
	"pablo@...filter.org" <pablo@...filter.org>,
	"netfilter-devel@...r.kernel.org" <netfilter-devel@...r.kernel.org>,
	"coreteam@...filter.org" <coreteam@...filter.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: "kadlec@...filter.org" <kadlec@...filter.org>, "fw@...len.de"
	<fw@...len.de>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org"
	<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"rkannoth@...vell.com" <rkannoth@...vell.com>, "wojciech.drewek@...el.com"
	<wojciech.drewek@...el.com>, "steen.hegenlund@...rohip.com"
	<steen.hegenlund@...rohip.com>, "keescook@...omium.org"
	<keescook@...omium.org>, Joao Moreira <joao.moreira@...el.com>
Subject: RE: [PATCH v2 2/2] Make num_actions unsigned

From: joao@...rdrivepizza.com
> Sent: 27 September 2023 03:02
> 
> From: Joao Moreira <joao.moreira@...el.com>
> 
> Currently, in nft_flow_rule_create function, num_actions is a signed
> integer. Yet, it is processed within a loop which increments its
> value. To prevent an overflow from occurring, make it unsigned and
> also check if it reaches UINT_MAX when being incremented.
> 
> After checking with maintainers, it was mentioned that front-end will
> cap the num_actions value and that it is not possible to reach such
> condition for an overflow. Yet, for correctness, it is still better to
> fix this.
> 
> This issue was observed by the commit author while reviewing a write-up
> regarding a CVE within the same subsystem [1].
> 
> 1 - https://nickgregory.me/post/2022/03/12/cve-2022-25636/
> 
> Signed-off-by: Joao Moreira <joao.moreira@...el.com>
> ---
>  net/netfilter/nf_tables_offload.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
> index 12ab78fa5d84..d25088791a74 100644
> --- a/net/netfilter/nf_tables_offload.c
> +++ b/net/netfilter/nf_tables_offload.c
> @@ -90,7 +90,8 @@ struct nft_flow_rule *nft_flow_rule_create(struct net *net,
>  {
>  	struct nft_offload_ctx *ctx;
>  	struct nft_flow_rule *flow;
> -	int num_actions = 0, err;
> +	unsigned int num_actions = 0;
> +	int err;
>  	struct nft_expr *expr;
> 
>  	expr = nft_expr_first(rule);
> @@ -99,6 +100,9 @@ struct nft_flow_rule *nft_flow_rule_create(struct net *net,
>  		    expr->ops->offload_action(expr))
>  			num_actions++;
> 
> +		if (num_actions == UINT_MAX)
> +			return ERR_PTR(-ENOMEM);
> +
>  		expr = nft_expr_next(expr);

The code is going to 'crash and burn' well before the counter
can possibly overflow.

nft_expr_next() is ((void *)expr) + expr->ops->size;

It is far more likely that has got setup wrong than the
count is too big. 

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)