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
| ||
|
Message-ID: <Y07raim32wOBRGPi@google.com> Date: Tue, 18 Oct 2022 11:07:38 -0700 From: sdf@...gle.com To: Kees Cook <keescook@...omium.org> Cc: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 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 <yhs@...com>, KP Singh <kpsingh@...nel.org>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, bpf@...r.kernel.org, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org Subject: Re: [PATCH] bpf: Use kmalloc_size_roundup() to match ksize() usage On 10/18, Kees Cook wrote: > Round up allocations with kmalloc_size_roundup() so that the verifier's > use of ksize() is always accurate and no special handling of the memory > is needed by KASAN, UBSAN_BOUNDS, nor FORTIFY_SOURCE. Pass the new size > information back up to callers so they can use the space immediately, > so array resizing to happen less frequently as well. Explicitly zero > any trailing bytes in new allocations. > Additionally fix a memory allocation leak: if krealloc() fails, "arr" > wasn't freed, but NULL was return to the caller of realloc_array() would > be writing NULL to the lvalue, losing the reference to the original > memory. > Cc: Alexei Starovoitov <ast@...nel.org> > Cc: Daniel Borkmann <daniel@...earbox.net> > Cc: John Fastabend <john.fastabend@...il.com> > Cc: Andrii Nakryiko <andrii@...nel.org> > Cc: Martin KaFai Lau <martin.lau@...ux.dev> > Cc: Song Liu <song@...nel.org> > Cc: Yonghong Song <yhs@...com> > Cc: KP Singh <kpsingh@...nel.org> > Cc: Stanislav Fomichev <sdf@...gle.com> > Cc: Hao Luo <haoluo@...gle.com> > Cc: Jiri Olsa <jolsa@...nel.org> > Cc: bpf@...r.kernel.org > Signed-off-by: Kees Cook <keescook@...omium.org> > --- > kernel/bpf/verifier.c | 49 +++++++++++++++++++++++++++---------------- > 1 file changed, 31 insertions(+), 18 deletions(-) > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 014ee0953dbd..8a0b60207d0e 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -1000,42 +1000,53 @@ static void print_insn_state(struct > bpf_verifier_env *env, > */ > static void *copy_array(void *dst, const void *src, size_t n, size_t > size, gfp_t flags) > { > - size_t bytes; > + size_t src_bytes, dst_bytes; > if (ZERO_OR_NULL_PTR(src)) > goto out; > - if (unlikely(check_mul_overflow(n, size, &bytes))) > + if (unlikely(check_mul_overflow(n, size, &src_bytes))) > return NULL; > - if (ksize(dst) < bytes) { > + dst_bytes = kmalloc_size_roundup(src_bytes); > + if (ksize(dst) < dst_bytes) { Why not simply do the following here? if (ksize(dst) < ksize(src)) { ? It seems like we care about src_bytes/bytes only in this case, so maybe move that check_mul_overflow under this branch as well? > kfree(dst); > - dst = kmalloc_track_caller(bytes, flags); > + dst = kmalloc_track_caller(dst_bytes, flags); > if (!dst) > return NULL; > } > - memcpy(dst, src, bytes); > + memcpy(dst, src, src_bytes); > + memset(dst + src_bytes, 0, dst_bytes - src_bytes); > out: > return dst ? dst : ZERO_SIZE_PTR; > } > -/* resize an array from old_n items to new_n items. the array is > reallocated if it's too > - * small to hold new_n items. new items are zeroed out if the array > grows. > +/* Resize an array from old_n items to *new_n items. The array is > reallocated if it's too > + * small to hold *new_n items. New items are zeroed out if the array > grows. Allocation > + * is rounded up to next kmalloc bucket size to reduce frequency of > resizing. *new_n > + * contains the new total number of items that will fit. > * > - * Contrary to krealloc_array, does not free arr if new_n is zero. > + * Contrary to krealloc, does not free arr if new_n is zero. > */ > -static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t > size) > +static void *realloc_array(void *arr, size_t old_n, size_t *new_n, > size_t size) > { > - if (!new_n || old_n == new_n) > + void *old_arr = arr; > + size_t alloc_size; > + > + if (!new_n || !*new_n || old_n == *new_n) > goto out; [..] > - arr = krealloc_array(arr, new_n, size, GFP_KERNEL); > - if (!arr) > + alloc_size = kmalloc_size_roundup(size_mul(*new_n, size)); > + arr = krealloc(old_arr, alloc_size, GFP_KERNEL); > + if (!arr) { > + kfree(old_arr); > return NULL; > + } Any reason not do hide this complexity behind krealloc_array? Why can't it take care of those roundup details? > - if (new_n > old_n) > - memset(arr + old_n * size, 0, (new_n - old_n) * size); > + *new_n = alloc_size / size; > + if (*new_n > old_n) > + memset(arr + old_n * size, 0, (*new_n - old_n) * size); > out: > return arr ? arr : ZERO_SIZE_PTR; > @@ -1067,7 +1078,7 @@ static int copy_stack_state(struct bpf_func_state > *dst, const struct bpf_func_st > static int resize_reference_state(struct bpf_func_state *state, size_t n) > { > - state->refs = realloc_array(state->refs, state->acquired_refs, n, > + state->refs = realloc_array(state->refs, state->acquired_refs, &n, > sizeof(struct bpf_reference_state)); > if (!state->refs) > return -ENOMEM; > @@ -1083,11 +1094,11 @@ static int grow_stack_state(struct bpf_func_state > *state, int size) > if (old_n >= n) > return 0; > - state->stack = realloc_array(state->stack, old_n, n, sizeof(struct > bpf_stack_state)); > + state->stack = realloc_array(state->stack, old_n, &n, sizeof(struct > bpf_stack_state)); > if (!state->stack) > return -ENOMEM; > - state->allocated_stack = size; > + state->allocated_stack = n * BPF_REG_SIZE; > return 0; > } > @@ -2499,9 +2510,11 @@ static int push_jmp_history(struct > bpf_verifier_env *env, > { > u32 cnt = cur->jmp_history_cnt; > struct bpf_idx_pair *p; > + size_t size; > cnt++; > - p = krealloc(cur->jmp_history, cnt * sizeof(*p), GFP_USER); > + size = kmalloc_size_roundup(size_mul(cnt, sizeof(*p))); > + p = krealloc(cur->jmp_history, size, GFP_USER); > if (!p) > return -ENOMEM; > p[cnt - 1].idx = env->insn_idx; > -- > 2.34.1
Powered by blists - more mailing lists