netdev - RE: [PATCH bpf] bpf: Fix DEVMAP_HASH overflow check on 32-bit arches

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65dfa50679d0a_2beb3208c8@john.notmuch>
Date: Wed, 28 Feb 2024 13:26:30 -0800
From: John Fastabend <john.fastabend@...il.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>, 
 Alexei Starovoitov <ast@...nel.org>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 "David S. Miller" <davem@...emloft.net>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Jesper Dangaard Brouer <hawk@...nel.org>, 
 John Fastabend <john.fastabend@...il.com>, 
 Andrii Nakryiko <andrii@...nel.org>, 
 Martin KaFai Lau <martin.lau@...ux.dev>, 
 Song Liu <song@...nel.org>, 
 Yonghong Song <yonghong.song@...ux.dev>, 
 KP Singh <kpsingh@...nel.org>, 
 Stanislav Fomichev <sdf@...gle.com>, 
 Hao Luo <haoluo@...gle.com>, 
 Jiri Olsa <jolsa@...nel.org>, 
 Toke Høiland-Jørgensen <toke@...hat.com>
Cc: syzbot+8cd36f6b65f3cafd400a@...kaller.appspotmail.com, 
 netdev@...r.kernel.org, 
 bpf@...r.kernel.org
Subject: RE: [PATCH bpf] bpf: Fix DEVMAP_HASH overflow check on 32-bit arches

Toke Høiland-Jørgensen wrote:
> 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@...kaller.appspotmail.com
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> ---
>  kernel/bpf/devmap.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index a936c704d4e7..9b2286f9c6da 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -130,13 +130,11 @@ 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 */
> +		if (dtab->map.max_entries > U32_MAX / 2)
>  			return -EINVAL;
> -	}
>  
> -	if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
> +		dtab->n_buckets = roundup_pow_of_two(dtab->map.max_entries);
> +
>  		dtab->dev_index_head = dev_map_create_hash(dtab->n_buckets,
>  							   dtab->map.numa_node);
>  		if (!dtab->dev_index_head)
> -- 
> 2.43.2
> 

I'm fairly sure this code was just taken from the hashtab implementation.
Do we also need a fix there?

        /* hash table size must be power of 2 */
        htab->n_buckets = roundup_pow_of_two(htab->map.max_entries);

The u32 check in hashtab is,

        /* prevent zero size kmalloc and check for u32 overflow */
        if (htab->n_buckets == 0 ||
            htab->n_buckets > U32_MAX / sizeof(struct bucket))
                goto free_htab;                 
                                  
Thanks,
John