NACK/Cmnt: [SRU][J][PATCH v2 1/1] netfilter: nf_tables: restore set elements when delete set fails
Manuel Diewald
manuel.diewald at canonical.com
Fri Aug 2 10:53:29 UTC 2024
On Thu, Aug 01, 2024 at 05:38:13PM +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 | 46 +++++++++++++++++++++++++++++-----
> 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, 45 insertions(+), 21 deletions(-)
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 0ba358207006..67b2dfb25f29 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,8 +624,8 @@ static void nft_map_catchall_deactivate(const struct nft_ctx *ctx,
> if (!nft_set_elem_active(ext, genmask))
> continue;
>
> - elem.priv = catchall->elem;
> - nft_setelem_data_deactivate(ctx->net, set, &elem);
> + nft_set_elem_change_active(ctx->net, set, ext);
> + nft_setelem_data_deactivate(ctx->net, set, catchall->elem);
I think this hunk merges code from upstream that we should not. The
function nft_setelem_data_deactivate() will access elem->priv, so you
cannot pass catchall->elem directly if I understand the code correctly.
It most likely still compiles fine because all the pointers involved
seem to be opaque void pointers. However, behavior might be unexpected
or incorrect. Changes to this code were introduced with
0e1ea651c971 netfilter: nf_tables: shrink memory consumption of set elements
which we do not carry in jammy, as you point out in your cover letter.
The patch should only add
+ nft_set_elem_change_active(ctx->net, set, ext);
and leave the call to nft_setelem_data_deactivate() unmodified, similar
to the upstream commit.
> break;
> }
> }
Thank you for addressing the things pointed out in the previous
submission!
Everything looks good now except for one small issue, see inline
comment.
--
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/20240802/7e092f2c/attachment-0001.sig>
More information about the kernel-team
mailing list