[PATCH 5.10 v2 0/3] Re-adapt "bpf: Fix DEVMAP_HASH overflow check on 32-bit arches"

[PATCH 5.10 v2 0/3] Re-adapt "bpf: Fix DEVMAP_HASH overflow check on 32-bit arches"

[Pu Lehui]

Commit 70294d8bc31f ("bpf: Eliminate rlimit-based memory accounting for
devmap maps") relies on the v5.11+ basic mechanism of memcg-based memory
accounting [0]. The commit cannot be independently backported to the
5.10 stable branch, otherwise the related memory when creating devmap
will be unrestricted and the associated bpf selftest map_ptr will fail.
Let's roll back to rlimit-based memory accounting mode for devmap and
re-adapt the commit 225da02acdc9 ("bpf: Fix DEVMAP_HASH overflow check
on 32-bit arches") to the 5.10 stable branch.

Link: https://lore.kernel.org/bpf/20201201215900.3569844-1-guro@fb.com [0]

Pu Lehui (2):
  Revert "bpf: Fix DEVMAP_HASH overflow check on 32-bit arches"
  Revert "bpf: Eliminate rlimit-based memory accounting for devmap maps"

Toke Høiland-Jørgensen (1):
  bpf: Fix DEVMAP_HASH overflow check on 32-bit arches

 kernel/bpf/devmap.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 5.10 v2 1/3] Revert "bpf: Fix DEVMAP_HASH overflow check on 32-bit arches"

[Pu Lehui]

From: Pu Lehui <pulehui@huawei.com>

This reverts commit 225da02acdc97af01b6bc6ce1a3e5362bf01d3fb.

Commit 225da02acdc9 ("bpf: fix DEVMAP_HASH overflow check on 32-bit
architectures") relies on the v5.11+ base mechanism of memcg-based
memory accounting[0], which is not yet supported on the 5.10 stable
branch, so let's revert this commit in preparation for re-adapting it.

Link: https://lore.kernel.org/bpf/20201201215900.3569844-1-guro@fb.com [0]
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 kernel/bpf/devmap.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 07b5edb2c70f..ca2cade2871b 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -129,14 +129,13 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
 	bpf_map_init_from_attr(&dtab->map, attr);
 
 	if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
-		/* hash table size must be power of 2; roundup_pow_of_two() can
-		 * overflow into UB on 32-bit arches, so check that first
-		 */
-		if (dtab->map.max_entries > 1UL << 31)
-			return -EINVAL;
-
 		dtab->n_buckets = roundup_pow_of_two(dtab->map.max_entries);
 
+		if (!dtab->n_buckets) /* Overflow check */
+			return -EINVAL;
+	}
+
+	if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
 		dtab->dev_index_head = dev_map_create_hash(dtab->n_buckets,
 							   dtab->map.numa_node);
 		if (!dtab->dev_index_head)
-- 
2.34.1

[PATCH 5.10 v2 2/3] Revert "bpf: Eliminate rlimit-based memory accounting for devmap maps"

[Pu Lehui]

From: Pu Lehui <pulehui@huawei.com>

This reverts commit 70294d8bc31f3b7789e5e32f757aa9344556d964.

Commit 70294d8bc31f ("bpf: Eliminate rlimit-based memory accounting for
devmap maps") is part of the v5.11+ base mechanism of memcg-based memory
accounting[0]. The commit cannot be independently backported to the 5.10
stable branch, otherwise the related memory when creating devmap will be
unrestricted. Let's roll back to rlimit-based memory accounting mode for
devmap.

Link: https://lore.kernel.org/bpf/20201201215900.3569844-1-guro@fb.com [0]
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 kernel/bpf/devmap.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index ca2cade2871b..01149821ded9 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -109,6 +109,8 @@ static inline struct hlist_head *dev_map_index_hash(struct bpf_dtab *dtab,
 static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
 {
 	u32 valsize = attr->value_size;
+	u64 cost = 0;
+	int err;
 
 	/* check sanity of attributes. 2 value sizes supported:
 	 * 4 bytes: ifindex
@@ -133,13 +135,21 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
 
 		if (!dtab->n_buckets) /* Overflow check */
 			return -EINVAL;
+		cost += (u64) sizeof(struct hlist_head) * dtab->n_buckets;
+	} else {
+		cost += (u64) dtab->map.max_entries * sizeof(struct bpf_dtab_netdev *);
 	}
 
+	/* if map size is larger than memlock limit, reject it */
+	err = bpf_map_charge_init(&dtab->map.memory, cost);
+	if (err)
+		return -EINVAL;
+
 	if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
 		dtab->dev_index_head = dev_map_create_hash(dtab->n_buckets,
 							   dtab->map.numa_node);
 		if (!dtab->dev_index_head)
-			return -ENOMEM;
+			goto free_charge;
 
 		spin_lock_init(&dtab->index_lock);
 	} else {
@@ -147,10 +157,14 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
 						      sizeof(struct bpf_dtab_netdev *),
 						      dtab->map.numa_node);
 		if (!dtab->netdev_map)
-			return -ENOMEM;
+			goto free_charge;
 	}
 
 	return 0;
+
+free_charge:
+	bpf_map_charge_finish(&dtab->map.memory);
+	return -ENOMEM;
 }
 
 static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
-- 
2.34.1

[PATCH 5.10 v2 3/3] bpf: Fix DEVMAP_HASH overflow check on 32-bit arches

[Pu Lehui]

From: Toke Høiland-Jørgensen <toke@redhat.com>

[ Upstream commit 281d464a34f540de166cee74b723e97ac2515ec3 ]

The devmap code allocates a number hash buckets equal to the next power
of two of the max_entries value provided when creating the map. When
rounding up to the next power of two, the 32-bit variable storing the
number of buckets can overflow, and the code checks for overflow by
checking if the truncated 32-bit value is equal to 0. However, on 32-bit
arches the rounding up itself can overflow mid-way through, because it
ends up doing a left-shift of 32 bits on an unsigned long value. If the
size of an unsigned long is four bytes, this is undefined behaviour, so
there is no guarantee that we'll end up with a nice and tidy 0-value at
the end.

Syzbot managed to turn this into a crash on arm32 by creating a
DEVMAP_HASH with max_entries > 0x80000000 and then trying to update it.
Fix this by moving the overflow check to before the rounding up
operation.

Fixes: 6f9d451ab1a3 ("xdp: Add devmap_hash map type for looking up devices by hashed index")
Link: https://lore.kernel.org/r/000000000000ed666a0611af6818@google.com
Reported-and-tested-by: syzbot+8cd36f6b65f3cafd400a@syzkaller.appspotmail.com
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
Message-ID: <20240307120340.99577-2-toke@redhat.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 kernel/bpf/devmap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 01149821ded9..7eb1282edc8e 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -131,10 +131,13 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
 	bpf_map_init_from_attr(&dtab->map, attr);
 
 	if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
-		dtab->n_buckets = roundup_pow_of_two(dtab->map.max_entries);
-
-		if (!dtab->n_buckets) /* Overflow check */
+		/* hash table size must be power of 2; roundup_pow_of_two() can
+		 * overflow into UB on 32-bit arches, so check that first
+		 */
+		if (dtab->map.max_entries > 1UL << 31)
 			return -EINVAL;
+
+		dtab->n_buckets = roundup_pow_of_two(dtab->map.max_entries);
 		cost += (u64) sizeof(struct hlist_head) * dtab->n_buckets;
 	} else {
 		cost += (u64) dtab->map.max_entries * sizeof(struct bpf_dtab_netdev *);
-- 
2.34.1

Re: [PATCH 5.10 v2 0/3] Re-adapt "bpf: Fix DEVMAP_HASH overflow check on 32-bit arches"

[Greg Kroah-Hartman]

On Fri, Sep 27, 2024 at 01:51:15PM +0000, Pu Lehui wrote:
> Commit 70294d8bc31f ("bpf: Eliminate rlimit-based memory accounting for
> devmap maps") relies on the v5.11+ basic mechanism of memcg-based memory
> accounting [0]. The commit cannot be independently backported to the
> 5.10 stable branch, otherwise the related memory when creating devmap
> will be unrestricted and the associated bpf selftest map_ptr will fail.
> Let's roll back to rlimit-based memory accounting mode for devmap and
> re-adapt the commit 225da02acdc9 ("bpf: Fix DEVMAP_HASH overflow check
> on 32-bit arches") to the 5.10 stable branch.
> 
> Link: https://lore.kernel.org/bpf/20201201215900.3569844-1-guro@fb.com [0]
> 
> Pu Lehui (2):
>   Revert "bpf: Fix DEVMAP_HASH overflow check on 32-bit arches"
>   Revert "bpf: Eliminate rlimit-based memory accounting for devmap maps"
> 
> Toke Høiland-Jørgensen (1):
>   bpf: Fix DEVMAP_HASH overflow check on 32-bit arches
> 
>  kernel/bpf/devmap.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> -- 
> 2.34.1
> 
> 

Now queud up, thanks.

greg k-h