| Message ID | 20210126082606.3183-1-minhquangbui99@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Fix integer overflow in argument calculation for bpf_map_area_alloc | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, 26 Jan 2021 at 08:26, Bui Quang Minh <minhquangbui99@gmail.com> wrote: > > In 32-bit architecture, the result of sizeof() is a 32-bit integer so > the expression becomes the multiplication between 2 32-bit integer which > can potentially leads to integer overflow. As a result, > bpf_map_area_alloc() allocates less memory than needed. > > Fix this by casting 1 operand to u64. Some quick thoughts: * Should this have a Fixes tag? * Seems like there are quite a few similar calls scattered around (cpumap, etc.). Did you audit these as well? * I'd prefer a calloc style version of bpf_map_area_alloc although that might conflict with Fixes tag. Lorenz
On Tue, Jan 26, 2021 at 09:36:57AM +0000, Lorenz Bauer wrote: > On Tue, 26 Jan 2021 at 08:26, Bui Quang Minh <minhquangbui99@gmail.com> wrote: > > > > In 32-bit architecture, the result of sizeof() is a 32-bit integer so > > the expression becomes the multiplication between 2 32-bit integer which > > can potentially leads to integer overflow. As a result, > > bpf_map_area_alloc() allocates less memory than needed. > > > > Fix this by casting 1 operand to u64. > > Some quick thoughts: > * Should this have a Fixes tag? Ok, I will add Fixes tag in later version patch. > * Seems like there are quite a few similar calls scattered around > (cpumap, etc.). Did you audit these as well? I spotted another bug after re-auditting. In hashtab, there ares 2 places using the same calls static struct bpf_map *htab_map_alloc(union bpf_attr *attr) { /* ... snip ... */ if (htab->n_buckets == 0 || htab->n_buckets > U32_MAX / sizeof(struct bucket)) goto free_htab; htab->buckets = bpf_map_area_alloc(htab->n_buckets * sizeof(struct bucket), htab->map.numa_node); } This is safe because of the above check. static int prealloc_init(struct bpf_htab *htab) { u32 num_entries = htab->map.max_entries; htab->elems = bpf_map_area_alloc(htab->elem_size * num_entries, htab->map.numa_node); } This is not safe since there is no limit check in elem_size. In cpumap, static struct bpf_map *cpu_map_alloc(union bpf_attr *attr) { cmap->cpu_map = bpf_map_area_alloc(cmap->map.max_entries * sizeof(struct bpf_cpu_map_entry *), cmap->map.numa_node); } I think this is safe because max_entries is not permitted to be larger than NR_CPUS. In stackmap, there is a place that I'm not very sure about static int prealloc_elems_and_freelist(struct bpf_stack_map *smap) { u32 elem_size = sizeof(struct stack_map_bucket) + smap->map.value_size; smap->elems = bpf_map_area_alloc(elem_size * smap->map.max_entries, smap->map.numa_node); } This is called after another bpf_map_area_alloc in stack_map_alloc(). In the first bpf_map_area_alloc() the argument is calculated in an u64 variable; so if in the second one, there is an integer overflow then the first one must be called with size > 4GB. I think the first one will probably fail (I am not sure about the actual limit of vmalloc()), so the second one might not be called. Overall, I think it is error prone in this pattern, maybe we should use typecasting in all similar calls or make a comment why we don't use typecasting. As I see typecasting is not so expensive and we can typecast the sizeof() operand so this change only affect 32-bit architecture. > * I'd prefer a calloc style version of bpf_map_area_alloc although > that might conflict with Fixes tag. Yes, I think the calloc style will prevent this kind of integer overflow bug. Thank you, Quang Minh.
On Wed, Jan 27, 2021 at 11:23:41AM +0700, Bui Quang Minh wrote: > > * Seems like there are quite a few similar calls scattered around > > (cpumap, etc.). Did you audit these as well? > > I spotted another bug after re-auditting. In hashtab, there ares 2 places using > the same calls > > static struct bpf_map *htab_map_alloc(union bpf_attr *attr) > { > /* ... snip ... */ > if (htab->n_buckets == 0 || > htab->n_buckets > U32_MAX / sizeof(struct bucket)) > goto free_htab; > > htab->buckets = bpf_map_area_alloc(htab->n_buckets * > sizeof(struct bucket), > htab->map.numa_node); > } > > This is safe because of the above check. > > static int prealloc_init(struct bpf_htab *htab) > { > u32 num_entries = htab->map.max_entries; > htab->elems = bpf_map_area_alloc(htab->elem_size * num_entries, > htab->map.numa_node); > } > > This is not safe since there is no limit check in elem_size. So sorry but I rechecked and saw this bug in hashtab has been fixed with commit e1868b9e36d0ca Thank you, Quang Minh.
On 1/27/21 5:23 AM, Bui Quang Minh wrote: > On Tue, Jan 26, 2021 at 09:36:57AM +0000, Lorenz Bauer wrote: >> On Tue, 26 Jan 2021 at 08:26, Bui Quang Minh <minhquangbui99@gmail.com> wrote: >>> >>> In 32-bit architecture, the result of sizeof() is a 32-bit integer so >>> the expression becomes the multiplication between 2 32-bit integer which >>> can potentially leads to integer overflow. As a result, >>> bpf_map_area_alloc() allocates less memory than needed. >>> >>> Fix this by casting 1 operand to u64. >> >> Some quick thoughts: >> * Should this have a Fixes tag? > > Ok, I will add Fixes tag in later version patch. > >> * Seems like there are quite a few similar calls scattered around >> (cpumap, etc.). Did you audit these as well? > [...] > In cpumap, > > static struct bpf_map *cpu_map_alloc(union bpf_attr *attr) > { > cmap->cpu_map = bpf_map_area_alloc(cmap->map.max_entries * > sizeof(struct bpf_cpu_map_entry *), > cmap->map.numa_node); > } > > I think this is safe because max_entries is not permitted to be larger than NR_CPUS. Yes. > In stackmap, there is a place that I'm not very sure about > > static int prealloc_elems_and_freelist(struct bpf_stack_map *smap) > { > u32 elem_size = sizeof(struct stack_map_bucket) + smap->map.value_size; > smap->elems = bpf_map_area_alloc(elem_size * smap->map.max_entries, > smap->map.numa_node); > } > > This is called after another bpf_map_area_alloc in stack_map_alloc(). In the first > bpf_map_area_alloc() the argument is calculated in an u64 variable; so if in the second > one, there is an integer overflow then the first one must be called with size > 4GB. I > think the first one will probably fail (I am not sure about the actual limit of vmalloc()), > so the second one might not be called. I would sanity check this as well. Looks like k*alloc()/v*alloc() call sites typically use array_size() which returns SIZE_MAX on overflow, 610b15c50e86 ("overflow.h: Add allocation size calculation helpers"). Thanks, Daniel
On 1/28/21 7:41 AM, Daniel Borkmann wrote: > On 1/27/21 5:23 AM, Bui Quang Minh wrote: >> On Tue, Jan 26, 2021 at 09:36:57AM +0000, Lorenz Bauer wrote: >>> On Tue, 26 Jan 2021 at 08:26, Bui Quang Minh >>> <minhquangbui99@gmail.com> wrote: >>>> >>>> In 32-bit architecture, the result of sizeof() is a 32-bit integer so >>>> the expression becomes the multiplication between 2 32-bit integer >>>> which >>>> can potentially leads to integer overflow. As a result, >>>> bpf_map_area_alloc() allocates less memory than needed. >>>> >>>> Fix this by casting 1 operand to u64. >>> >>> Some quick thoughts: >>> * Should this have a Fixes tag? >> >> Ok, I will add Fixes tag in later version patch. >> >>> * Seems like there are quite a few similar calls scattered around >>> (cpumap, etc.). Did you audit these as well? >> > [...] >> In cpumap, >> >> static struct bpf_map *cpu_map_alloc(union bpf_attr *attr) >> { >> cmap->cpu_map = bpf_map_area_alloc(cmap->map.max_entries * >> sizeof(struct bpf_cpu_map_entry *), >> cmap->map.numa_node); >> } >> >> I think this is safe because max_entries is not permitted to be larger >> than NR_CPUS. > > Yes. > >> In stackmap, there is a place that I'm not very sure about >> >> static int prealloc_elems_and_freelist(struct bpf_stack_map *smap) >> { >> u32 elem_size = sizeof(struct stack_map_bucket) + >> smap->map.value_size; >> smap->elems = bpf_map_area_alloc(elem_size * >> smap->map.max_entries, >> smap->map.numa_node); >> } >> >> This is called after another bpf_map_area_alloc in stack_map_alloc(). >> In the first >> bpf_map_area_alloc() the argument is calculated in an u64 variable; so >> if in the second >> one, there is an integer overflow then the first one must be called >> with size > 4GB. I >> think the first one will probably fail (I am not sure about the actual >> limit of vmalloc()), >> so the second one might not be called. > > I would sanity check this as well. Looks like k*alloc()/v*alloc() call > sites typically > use array_size() which returns SIZE_MAX on overflow, 610b15c50e86 > ("overflow.h: Add > allocation size calculation helpers"). Hi, I almost forget about this patch, I have checked the bpf_map_area_alloc in in stackmap.c and I can see that integer overflow cannot happen in this stackmap.c case. In stack_map_alloc(), u64 cost; ... cost = n_buckets * sizeof(struct stack_map_bucket *) + sizeof(*smap); cost += n_buckets * (value_size + sizeof(struct stack_map_bucket)); smap = bpf_map_area_alloc(cost, bpf_map_attr_numa_node(attr)); (1) ... prealloc_elems_and_freelist(smap); In prealloc_elems_and_freelist(), u32 elem_size = sizeof(struct stack_map_bucket) + smap->map.value_size; smap->elems = bpf_map_area_alloc(elem_size * smap->map.max_entries, smap->map.numa_node); (2) Argument calculation at (1) is safe. Argument calculation at (2) can potentially result in an integer overflow in 32-bit architecture. However, if the integer overflow happens, it means argument at (1) must be 2**32, which cannot pass the SIZE_MAX check in __bpf_map_area_alloc() In __bpf_map_area_alloc() if (size >= SIZE_MAX) return NULL; So I think the original patch has fixed instances of this bug pattern. Thank you, Quang Minh.
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index f6e9c68afdd4..e849c3e8a49f 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -92,7 +92,7 @@ static struct hlist_head *dev_map_create_hash(unsigned int entries, int i; struct hlist_head *hash; - hash = bpf_map_area_alloc(entries * sizeof(*hash), numa_node); + hash = bpf_map_area_alloc((u64) entries * sizeof(*hash), numa_node); if (hash != NULL) for (i = 0; i < entries; i++) INIT_HLIST_HEAD(&hash[i]); @@ -143,7 +143,7 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr) spin_lock_init(&dtab->index_lock); } else { - dtab->netdev_map = bpf_map_area_alloc(dtab->map.max_entries * + dtab->netdev_map = bpf_map_area_alloc((u64) dtab->map.max_entries * sizeof(struct bpf_dtab_netdev *), dtab->map.numa_node); if (!dtab->netdev_map) diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 64b5ec14ff50..7a42016a981d 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -44,7 +44,7 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr) bpf_map_init_from_attr(&stab->map, attr); raw_spin_lock_init(&stab->lock); - stab->sks = bpf_map_area_alloc(stab->map.max_entries * + stab->sks = bpf_map_area_alloc((u64) stab->map.max_entries * sizeof(struct sock *), stab->map.numa_node); if (!stab->sks) {
In 32-bit architecture, the result of sizeof() is a 32-bit integer so the expression becomes the multiplication between 2 32-bit integer which can potentially leads to integer overflow. As a result, bpf_map_area_alloc() allocates less memory than needed. Fix this by casting 1 operand to u64. Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com> --- kernel/bpf/devmap.c | 4 ++-- net/core/sock_map.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)