RE: [PATCH v2 2/2] Make num_actions unsigned

[David Laight]

From: joao@xxxxxxxxxxxxxxxxxx
> Sent: 27 September 2023 03:02
> 
> From: Joao Moreira <joao.moreira@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> ---
>  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)