netfilter: nft_set_pipapo: do not free live element

jira LE-2169
cve CVE-2024-26924
Rebuild_History Non-Buildable kernel-4.18.0-553.27.1.el8_10
commit-author Florian Westphal <[email protected]>
commit 3cfc9ec
Empty-Commit: Cherry-Pick Conflicts during history rebuild.
Will be included in final tarball splat. Ref for failed cherry-pick at:
ciq/ciq_backports/kernel-4.18.0-553.27.1.el8_10/3cfc9ec0.failed

Pablo reports a crash with large batches of elements with a
back-to-back add/remove pattern.  Quoting Pablo:

  add_elem("00000000") timeout 100 ms
  ...
  add_elem("0000000X") timeout 100 ms
  del_elem("0000000X") <---------------- delete one that was just added
  ...
  add_elem("00005000") timeout 100 ms

  1) nft_pipapo_remove() removes element 0000000X
  Then, KASAN shows a splat.

Looking at the remove function there is a chance that we will drop a
rule that maps to a non-deactivated element.

Removal happens in two steps, first we do a lookup for key k and return the
to-be-removed element and mark it as inactive in the next generation.
Then, in a second step, the element gets removed from the set/map.

The _remove function does not work correctly if we have more than one
element that share the same key.

This can happen if we insert an element into a set when the set already
holds an element with same key, but the element mapping to the existing
key has timed out or is not active in the next generation.

In such case its possible that removal will unmap the wrong element.
If this happens, we will leak the non-deactivated element, it becomes
unreachable.

The element that got deactivated (and will be freed later) will
remain reachable in the set data structure, this can result in
a crash when such an element is retrieved during lookup (stale
pointer).

Add a check that the fully matching key does in fact map to the element
that we have marked as inactive in the deactivation step.
If not, we need to continue searching.

Add a bug/warn trap at the end of the function as well, the remove
function must not ever be called with an invisible/unreachable/non-existent
element.

v2: avoid uneeded temporary variable (Stefano)

Fixes: 3c4287f ("nf_tables: Add set type for arbitrary concatenation of ranges")
	Reported-by: Pablo Neira Ayuso <[email protected]>
	Reviewed-by: Stefano Brivio <[email protected]>
	Signed-off-by: Florian Westphal <[email protected]>
	Signed-off-by: Pablo Neira Ayuso <[email protected]>
(cherry picked from commit 3cfc9ec)
	Signed-off-by: Jonathan Maple <[email protected]>

# Conflicts:
#	net/netfilter/nft_set_pipapo.c

Author: PlaidCat

ciq/ciq_backports/kernel-4.18.0-553.27.1.el8_10/3cfc9ec0.failed

@@ -0,0 +1,98 @@
+netfilter: nft_set_pipapo: do not free live element
+
+jira LE-2169
+cve CVE-2024-26924
+Rebuild_History Non-Buildable kernel-4.18.0-553.27.1.el8_10
+commit-author Florian Westphal <[email protected]>
+commit 3cfc9ec039af60dbd8965ae085b2c2ccdcfbe1cc
+Empty-Commit: Cherry-Pick Conflicts during history rebuild.
+Will be included in final tarball splat. Ref for failed cherry-pick at:
+ciq/ciq_backports/kernel-4.18.0-553.27.1.el8_10/3cfc9ec0.failed
+
+Pablo reports a crash with large batches of elements with a
+back-to-back add/remove pattern.  Quoting Pablo:
+
+  add_elem("00000000") timeout 100 ms
+  ...
+  add_elem("0000000X") timeout 100 ms
+  del_elem("0000000X") <---------------- delete one that was just added
+  ...
+  add_elem("00005000") timeout 100 ms
+
+  1) nft_pipapo_remove() removes element 0000000X
+  Then, KASAN shows a splat.
+
+Looking at the remove function there is a chance that we will drop a
+rule that maps to a non-deactivated element.
+
+Removal happens in two steps, first we do a lookup for key k and return the
+to-be-removed element and mark it as inactive in the next generation.
+Then, in a second step, the element gets removed from the set/map.
+
+The _remove function does not work correctly if we have more than one
+element that share the same key.
+
+This can happen if we insert an element into a set when the set already
+holds an element with same key, but the element mapping to the existing
+key has timed out or is not active in the next generation.
+
+In such case its possible that removal will unmap the wrong element.
+If this happens, we will leak the non-deactivated element, it becomes
+unreachable.
+
+The element that got deactivated (and will be freed later) will
+remain reachable in the set data structure, this can result in
+a crash when such an element is retrieved during lookup (stale
+pointer).
+
+Add a check that the fully matching key does in fact map to the element
+that we have marked as inactive in the deactivation step.
+If not, we need to continue searching.
+
+Add a bug/warn trap at the end of the function as well, the remove
+function must not ever be called with an invisible/unreachable/non-existent
+element.
+
+v2: avoid uneeded temporary variable (Stefano)
+
+Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
+	Reported-by: Pablo Neira Ayuso <[email protected]>
+	Reviewed-by: Stefano Brivio <[email protected]>
+	Signed-off-by: Florian Westphal <[email protected]>
+	Signed-off-by: Pablo Neira Ayuso <[email protected]>
+(cherry picked from commit 3cfc9ec039af60dbd8965ae085b2c2ccdcfbe1cc)
+	Signed-off-by: Jonathan Maple <[email protected]>
+
+# Conflicts:
+#	net/netfilter/nft_set_pipapo.c
+diff --cc net/netfilter/nft_set_pipapo.c
+index 4b6a6667d72b,eeaf05ffba95..000000000000
+--- a/net/netfilter/nft_set_pipapo.c
++++ b/net/netfilter/nft_set_pipapo.c
+@@@ -1880,14 -2089,14 +1882,20 @@@ static void nft_pipapo_remove(const str
+  			rules_fx = f->mt[start].n;
+  			start = f->mt[start].to;
+  
+++<<<<<<< HEAD
+ +			match_start += NFT_PIPAPO_GROUPS_PADDED_SIZE(f->groups);
+ +			match_end += NFT_PIPAPO_GROUPS_PADDED_SIZE(f->groups);
+ +		}
+- 
+- 		if (i == m->field_count) {
+- 			priv->dirty = true;
+- 			pipapo_drop(m, rulemap);
+- 			return;
+++=======
++ 			match_start += NFT_PIPAPO_GROUPS_PADDED_SIZE(f);
++ 			match_end += NFT_PIPAPO_GROUPS_PADDED_SIZE(f);
+++>>>>>>> 3cfc9ec039af (netfilter: nft_set_pipapo: do not free live element)
++ 
++ 			if (last && f->mt[rulemap[i].to].e == e) {
++ 				priv->dirty = true;
++ 				pipapo_drop(m, rulemap);
++ 				return;
++ 			}
+  		}
+  
+  		first_rule += rules_f0;
+* Unmerged path net/netfilter/nft_set_pipapo.c