NACK/Cmnt: [SRU][J][PATCH v3 1/1] netfilter: nf_tables: restore set elements when delete set fails

[Manuel Diewald]

On Tue, Aug 06, 2024 at 01:40:58PM +0200, Hannah Peuckmann wrote:
> From: Pablo Neira Ayuso <pablo at netfilter.org>
> 
> From abort path, nft_mapelem_activate() needs to restore refcounters to
> the original state. Currently, it uses the set->ops->walk() to iterate
> over these set elements. The existing set iterator skips inactive
> elements in the next generation, this does not work from the abort path
> to restore the original state since it has to skip active elements
> instead (not inactive ones).
> 
> This patch moves the check for inactive elements to the set iterator
> callback, then it reverses the logic for the .activate case which
> needs to skip active elements.
> 
> Toggle next generation bit for elements when delete set command is
> invoked and call nft_clear() from .activate (abort) path to restore the
> next generation bit.
> 
> The splat below shows an object in mappings memleak:
> 
> [43929.457523] ------------[ cut here ]------------
> [43929.457532] WARNING: CPU: 0 PID: 1139 at include/net/netfilter/nf_tables.h:1237 nft_setelem_data_deactivate+0xe4/0xf0 [nf_tables]
> [...]
> [43929.458014] RIP: 0010:nft_setelem_data_deactivate+0xe4/0xf0 [nf_tables]
> [43929.458076] Code: 83 f8 01 77 ab 49 8d 7c 24 08 e8 37 5e d0 de 49 8b 6c 24 08 48 8d 7d 50 e8 e9 5c d0 de 8b 45 50 8d 50 ff 89 55 50 85 c0 75 86 <0f> 0b eb 82 0f 0b eb b3 0f 1f 40 00 90 90 90 90 90 90 90 90 90 90
> [43929.458081] RSP: 0018:ffff888140f9f4b0 EFLAGS: 00010246
> [43929.458086] RAX: 0000000000000000 RBX: ffff8881434f5288 RCX: dffffc0000000000
> [43929.458090] RDX: 00000000ffffffff RSI: ffffffffa26d28a7 RDI: ffff88810ecc9550
> [43929.458093] RBP: ffff88810ecc9500 R08: 0000000000000001 R09: ffffed10281f3e8f
> [43929.458096] R10: 0000000000000003 R11: ffff0000ffff0000 R12: ffff8881434f52a0
> [43929.458100] R13: ffff888140f9f5f4 R14: ffff888151c7a800 R15: 0000000000000002
> [43929.458103] FS:  00007f0c687c4740(0000) GS:ffff888390800000(0000) knlGS:0000000000000000
> [43929.458107] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [43929.458111] CR2: 00007f58dbe5b008 CR3: 0000000123602005 CR4: 00000000001706f0
> [43929.458114] Call Trace:
> [43929.458118]  <TASK>
> [43929.458121]  ? __warn+0x9f/0x1a0
> [43929.458127]  ? nft_setelem_data_deactivate+0xe4/0xf0 [nf_tables]
> [43929.458188]  ? report_bug+0x1b1/0x1e0
> [43929.458196]  ? handle_bug+0x3c/0x70
> [43929.458200]  ? exc_invalid_op+0x17/0x40
> [43929.458211]  ? nft_setelem_data_deactivate+0xd7/0xf0 [nf_tables]
> [43929.458271]  ? nft_setelem_data_deactivate+0xe4/0xf0 [nf_tables]
> [43929.458332]  nft_mapelem_deactivate+0x24/0x30 [nf_tables]
> [43929.458392]  nft_rhash_walk+0xdd/0x180 [nf_tables]
> [43929.458453]  ? __pfx_nft_rhash_walk+0x10/0x10 [nf_tables]
> [43929.458512]  ? rb_insert_color+0x2e/0x280
> [43929.458520]  nft_map_deactivate+0xdc/0x1e0 [nf_tables]
> [43929.458582]  ? __pfx_nft_map_deactivate+0x10/0x10 [nf_tables]
> [43929.458642]  ? __pfx_nft_mapelem_deactivate+0x10/0x10 [nf_tables]
> [43929.458701]  ? __rcu_read_unlock+0x46/0x70
> [43929.458709]  nft_delset+0xff/0x110 [nf_tables]
> [43929.458769]  nft_flush_table+0x16f/0x460 [nf_tables]
> [43929.458830]  nf_tables_deltable+0x501/0x580 [nf_tables]
> 
> Fixes: 628bd3e49cba ("netfilter: nf_tables: drop map element references from preparation phase")
> Signed-off-by: Pablo Neira Ayuso <pablo at netfilter.org>
> 
> (backported from commit e79b47a8615d42c68aaeb68971593333667382ed linux-6.9.y)
> [hannsofie: context adjustments in nf_tables_api.c and nft_set_pipapo.c]
> CVE-2024-27012
> Signed-off-by: Hannah Peuckmann <hannah.peuckmann at canonical.com>
> ---
>  net/netfilter/nf_tables_api.c  | 44 ++++++++++++++++++++++++++++++----
>  net/netfilter/nft_set_bitmap.c |  4 +---
>  net/netfilter/nft_set_hash.c   |  8 ++-----
>  net/netfilter/nft_set_pipapo.c |  4 +---
>  net/netfilter/nft_set_rbtree.c |  4 +---
>  5 files changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 0ba358207006..95704b087f1d 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -594,6 +594,12 @@ static int nft_mapelem_deactivate(const struct nft_ctx *ctx,
>  				  const struct nft_set_iter *iter,
>  				  struct nft_set_elem *elem)
>  {
> +	struct nft_set_ext *ext = nft_set_elem_ext(set, elem);
> +
> +	if (!nft_set_elem_active(ext, iter->genmask))
> +		return 0;
> +
> +	nft_set_elem_change_active(ctx->net, set, ext);
>  	nft_setelem_data_deactivate(ctx->net, set, elem);
>  
>  	return 0;
> @@ -618,7 +624,7 @@ static void nft_map_catchall_deactivate(const struct nft_ctx *ctx,
>  		if (!nft_set_elem_active(ext, genmask))
>  			continue;
>  
> -		elem.priv = catchall->elem;

Sorry, I think my comment on v2 of the patch might have been unclear
about this. We need to keep this line, too. Similar to the upstream
commit, I think we should only add

+             nft_set_elem_change_active(ctx->net, set, ext);

and leave the surrounding context as is. Not setting elem.priv will lead
to problems, I would assume.

> +		nft_set_elem_change_active(ctx->net, set, ext);
>  		nft_setelem_data_deactivate(ctx->net, set, &elem);
>  		break;
>  	}

As a general note, it can be helpful for reviewers if you briefly
describe the changes between v1 -> v2 -> v3, for example in the cover
letter.

-- 
 Manuel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20240806/0b264d71/attachment.sig>