* [PATCH bpf-next v2 1/3] bpf/verifier: Fix potential memory leak in array reallocation
2022-10-29 2:54 [PATCH bpf-next v2 0/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage Kees Cook
@ 2022-10-29 2:54 ` Kees Cook
2022-10-31 20:16 ` Bill Wendling
2022-10-29 2:54 ` [PATCH bpf-next v2 2/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage Kees Cook
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2022-10-29 2:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kees Cook, Daniel Borkmann, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, linux-kernel,
linux-hardening
If an error (NULL) is returned by krealloc(), callers of realloc_array()
were setting their allocation pointers to NULL, but on error krealloc()
does not touch the original allocation. This would result in a memory
resource leak. Instead, free the old allocation on the error handling
path.
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Song Liu <song@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: bpf@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
kernel/bpf/verifier.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 014ee0953dbd..eb8c34db74c7 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1027,12 +1027,17 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
*/
static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t size)
{
+ void *new_arr;
+
if (!new_n || old_n == new_n)
goto out;
- arr = krealloc_array(arr, new_n, size, GFP_KERNEL);
- if (!arr)
+ new_arr = krealloc_array(arr, new_n, size, GFP_KERNEL);
+ if (!new_arr) {
+ kfree(arr);
return NULL;
+ }
+ arr = new_arr;
if (new_n > old_n)
memset(arr + old_n * size, 0, (new_n - old_n) * size);
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v2 1/3] bpf/verifier: Fix potential memory leak in array reallocation
2022-10-29 2:54 ` [PATCH bpf-next v2 1/3] bpf/verifier: Fix potential memory leak in array reallocation Kees Cook
@ 2022-10-31 20:16 ` Bill Wendling
2022-11-01 13:46 ` Daniel Borkmann
0 siblings, 1 reply; 12+ messages in thread
From: Bill Wendling @ 2022-10-31 20:16 UTC (permalink / raw)
To: Kees Cook
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf,
linux-kernel, linux-hardening
On Fri, Oct 28, 2022 at 7:55 PM Kees Cook <keescook@chromium.org> wrote:
>
> If an error (NULL) is returned by krealloc(), callers of realloc_array()
> were setting their allocation pointers to NULL, but on error krealloc()
> does not touch the original allocation. This would result in a memory
> resource leak. Instead, free the old allocation on the error handling
> path.
>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Martin KaFai Lau <martin.lau@linux.dev>
> Cc: Song Liu <song@kernel.org>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: KP Singh <kpsingh@kernel.org>
> Cc: Stanislav Fomichev <sdf@google.com>
> Cc: Hao Luo <haoluo@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: bpf@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Bill Wendling <morbo@google.com>
> ---
> kernel/bpf/verifier.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 014ee0953dbd..eb8c34db74c7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1027,12 +1027,17 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
> */
> static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t size)
> {
> + void *new_arr;
> +
> if (!new_n || old_n == new_n)
> goto out;
>
> - arr = krealloc_array(arr, new_n, size, GFP_KERNEL);
> - if (!arr)
> + new_arr = krealloc_array(arr, new_n, size, GFP_KERNEL);
> + if (!new_arr) {
> + kfree(arr);
> return NULL;
> + }
> + arr = new_arr;
>
> if (new_n > old_n)
> memset(arr + old_n * size, 0, (new_n - old_n) * size);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v2 1/3] bpf/verifier: Fix potential memory leak in array reallocation
2022-10-31 20:16 ` Bill Wendling
@ 2022-11-01 13:46 ` Daniel Borkmann
2022-11-15 16:07 ` Lorenz Bauer
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Borkmann @ 2022-11-01 13:46 UTC (permalink / raw)
To: Bill Wendling, Kees Cook
Cc: Alexei Starovoitov, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, linux-kernel,
linux-hardening, Zhengchao Shao, Lorenz Bauer
[ +Lorenz ]
On 10/31/22 9:16 PM, Bill Wendling wrote:
> On Fri, Oct 28, 2022 at 7:55 PM Kees Cook <keescook@chromium.org> wrote:
>>
>> If an error (NULL) is returned by krealloc(), callers of realloc_array()
>> were setting their allocation pointers to NULL, but on error krealloc()
>> does not touch the original allocation. This would result in a memory
>> resource leak. Instead, free the old allocation on the error handling
>> path.
>>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: John Fastabend <john.fastabend@gmail.com>
>> Cc: Andrii Nakryiko <andrii@kernel.org>
>> Cc: Martin KaFai Lau <martin.lau@linux.dev>
>> Cc: Song Liu <song@kernel.org>
>> Cc: Yonghong Song <yhs@fb.com>
>> Cc: KP Singh <kpsingh@kernel.org>
>> Cc: Stanislav Fomichev <sdf@google.com>
>> Cc: Hao Luo <haoluo@google.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: bpf@vger.kernel.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Reviewed-by: Bill Wendling <morbo@google.com>
>
>> ---
>> kernel/bpf/verifier.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 014ee0953dbd..eb8c34db74c7 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -1027,12 +1027,17 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
>> */
>> static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t size)
>> {
>> + void *new_arr;
>> +
>> if (!new_n || old_n == new_n)
>> goto out;
>>
>> - arr = krealloc_array(arr, new_n, size, GFP_KERNEL);
>> - if (!arr)
>> + new_arr = krealloc_array(arr, new_n, size, GFP_KERNEL);
>> + if (!new_arr) {
>> + kfree(arr);
>> return NULL;
>> + }
>> + arr = new_arr;
Fyi, I took this fix into bpf tree and improved commit log a bit with the
one from Zhengchao [0] given yours came in first. Fixes tag would have been
nice, I added the c69431aab67a to the commit message while applying.
[0] https://patchwork.kernel.org/project/netdevbpf/patch/20221031070812.339883-1-shaozhengchao@huawei.com/
>> if (new_n > old_n)
>> memset(arr + old_n * size, 0, (new_n - old_n) * size);
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next v2 2/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage
2022-10-29 2:54 [PATCH bpf-next v2 0/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage Kees Cook
2022-10-29 2:54 ` [PATCH bpf-next v2 1/3] bpf/verifier: Fix potential memory leak in array reallocation Kees Cook
@ 2022-10-29 2:54 ` Kees Cook
2022-11-01 13:52 ` Daniel Borkmann
2022-10-29 2:54 ` [PATCH bpf-next v2 3/3] bpf/verifier: Take advantage of full allocation sizes Kees Cook
2022-11-01 13:40 ` [PATCH bpf-next v2 0/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage patchwork-bot+netdevbpf
3 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2022-10-29 2:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kees Cook, Daniel Borkmann, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, linux-kernel,
linux-hardening
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.
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Song Liu <song@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: bpf@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
kernel/bpf/verifier.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index eb8c34db74c7..1c040d27b8f6 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1008,9 +1008,9 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
if (unlikely(check_mul_overflow(n, size, &bytes)))
return NULL;
- if (ksize(dst) < bytes) {
+ if (ksize(dst) < ksize(src)) {
kfree(dst);
- dst = kmalloc_track_caller(bytes, flags);
+ dst = kmalloc_track_caller(kmalloc_size_roundup(bytes), flags);
if (!dst)
return NULL;
}
@@ -1027,12 +1027,14 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
*/
static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t size)
{
+ size_t alloc_size;
void *new_arr;
if (!new_n || old_n == new_n)
goto out;
- new_arr = krealloc_array(arr, new_n, size, GFP_KERNEL);
+ alloc_size = kmalloc_size_roundup(size_mul(new_n, size));
+ new_arr = krealloc(arr, alloc_size, GFP_KERNEL);
if (!new_arr) {
kfree(arr);
return NULL;
@@ -2504,9 +2506,11 @@ static int push_jmp_history(struct bpf_verifier_env *env,
{
u32 cnt = cur->jmp_history_cnt;
struct bpf_idx_pair *p;
+ size_t alloc_size;
cnt++;
- p = krealloc(cur->jmp_history, cnt * sizeof(*p), GFP_USER);
+ alloc_size = kmalloc_size_roundup(size_mul(cnt, sizeof(*p)));
+ p = krealloc(cur->jmp_history, alloc_size, GFP_USER);
if (!p)
return -ENOMEM;
p[cnt - 1].idx = env->insn_idx;
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v2 2/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage
2022-10-29 2:54 ` [PATCH bpf-next v2 2/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage Kees Cook
@ 2022-11-01 13:52 ` Daniel Borkmann
2022-11-01 17:01 ` Kees Cook
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Borkmann @ 2022-11-01 13:52 UTC (permalink / raw)
To: Kees Cook, Alexei Starovoitov
Cc: John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
bpf, linux-kernel, linux-hardening
On 10/29/22 4:54 AM, 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.
>
[...]
The commit message is a bit cryptic here without further context. Is this
a bug fix or improvement? I read the latter, but it would be good to have
more context here for reviewers (maybe Link tag pointing to some discussion
or the like). Also, why is the kmalloc_size_roundup() not hidden for kmalloc
callers, isn't this a tree-wide issue?
Thanks,
Daniel
> kernel/bpf/verifier.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index eb8c34db74c7..1c040d27b8f6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1008,9 +1008,9 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
> if (unlikely(check_mul_overflow(n, size, &bytes)))
> return NULL;
>
> - if (ksize(dst) < bytes) {
> + if (ksize(dst) < ksize(src)) {
> kfree(dst);
> - dst = kmalloc_track_caller(bytes, flags);
> + dst = kmalloc_track_caller(kmalloc_size_roundup(bytes), flags);
> if (!dst)
> return NULL;
> }
> @@ -1027,12 +1027,14 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
> */
> static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t size)
> {
> + size_t alloc_size;
> void *new_arr;
>
> if (!new_n || old_n == new_n)
> goto out;
>
> - new_arr = krealloc_array(arr, new_n, size, GFP_KERNEL);
> + alloc_size = kmalloc_size_roundup(size_mul(new_n, size));
> + new_arr = krealloc(arr, alloc_size, GFP_KERNEL);
> if (!new_arr) {
> kfree(arr);
> return NULL;
> @@ -2504,9 +2506,11 @@ static int push_jmp_history(struct bpf_verifier_env *env,
> {
> u32 cnt = cur->jmp_history_cnt;
> struct bpf_idx_pair *p;
> + size_t alloc_size;
>
> cnt++;
> - p = krealloc(cur->jmp_history, cnt * sizeof(*p), GFP_USER);
> + alloc_size = kmalloc_size_roundup(size_mul(cnt, sizeof(*p)));
> + p = krealloc(cur->jmp_history, alloc_size, GFP_USER);
> if (!p)
> return -ENOMEM;
> p[cnt - 1].idx = env->insn_idx;
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v2 2/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage
2022-11-01 13:52 ` Daniel Borkmann
@ 2022-11-01 17:01 ` Kees Cook
0 siblings, 0 replies; 12+ messages in thread
From: Kees Cook @ 2022-11-01 17:01 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, linux-kernel,
linux-hardening
On Tue, Nov 01, 2022 at 02:52:16PM +0100, Daniel Borkmann wrote:
> On 10/29/22 4:54 AM, 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.
> >
> [...]
>
> The commit message is a bit cryptic here without further context. Is this
> a bug fix or improvement? I read the latter, but it would be good to have
It's an improvement -- e.g. it depends on the recently added
kmalloc_size_roundup() helper.
> more context here for reviewers (maybe Link tag pointing to some discussion
> or the like). Also, why is the kmalloc_size_roundup() not hidden for kmalloc
> callers, isn't this a tree-wide issue?
The main issue is that _most_ allocation callers want an explicitly sized
allocation (and not "more"), and that dynamic runtime analysis tools
(e.g. KASAN, UBSAN_BOUNDS, FORTIFY_SOURCE, etc) are looking for precise
bounds checking (i.e. not something that is rounded up). A tiny handful
of allocations were doing an implicit alloc/realloc loop that actually
depended on ksize(), and didn't actually always call realloc. This has
created a long series of bugs and problems over many years related to the
runtime bounds checking, so these callers are finally being adjusted to
_not_ depend on the ksize() side-effect, by doing one of several things:
- tracking the allocation size precisely and just never calling ksize()
at all[1].
- always calling realloc and not using ksize() at all. (This solution
ends up actually be a subset of the next solution.)
- using kmalloc_size_roundup() to explicitly round up the desired
allocation size immediately[2].
The bpf/verifier case is this another of this latter case.
Because some of the dynamic bounds checking depends on the size being an
_argument_ to an allocator function (i.e. see the __alloc_size attribute),
the ksize() users are rare, and it could waste local variables, it
was been deemed better to explicitly separate the rounding up from the
allocation itself[3].
Hopefully that helps clarify! :)
-Kees
[1] e.g.:
https://git.kernel.org/linus/712f210a457d
https://git.kernel.org/linus/72c08d9f4c72
[2] e.g.:
https://git.kernel.org/netdev/net-next/c/12d6c1d3a2ad
https://git.kernel.org/netdev/net-next/c/ab3f7828c979
https://git.kernel.org/netdev/net-next/c/d6dd508080a3
[3] https://lore.kernel.org/lkml/0ea1fc165a6c6117f982f4f135093e69cb884930.camel@redhat.com/
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next v2 3/3] bpf/verifier: Take advantage of full allocation sizes
2022-10-29 2:54 [PATCH bpf-next v2 0/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage Kees Cook
2022-10-29 2:54 ` [PATCH bpf-next v2 1/3] bpf/verifier: Fix potential memory leak in array reallocation Kees Cook
2022-10-29 2:54 ` [PATCH bpf-next v2 2/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage Kees Cook
@ 2022-10-29 2:54 ` Kees Cook
2022-10-31 21:53 ` Stanislav Fomichev
2022-11-01 13:40 ` [PATCH bpf-next v2 0/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage patchwork-bot+netdevbpf
3 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2022-10-29 2:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kees Cook, Daniel Borkmann, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, linux-kernel,
linux-hardening
Since the full kmalloc bucket size is being explicitly allocated, pass
back the resulting details to take advantage of the full size so that
reallocation checking will be needed less frequently.
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Song Liu <song@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: bpf@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
kernel/bpf/verifier.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1c040d27b8f6..e58b554e862b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1020,20 +1020,23 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
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)
{
size_t alloc_size;
void *new_arr;
- if (!new_n || old_n == new_n)
+ if (!new_n || !*new_n || old_n == *new_n)
goto out;
- alloc_size = kmalloc_size_roundup(size_mul(new_n, size));
+ alloc_size = kmalloc_size_roundup(size_mul(*new_n, size));
new_arr = krealloc(arr, alloc_size, GFP_KERNEL);
if (!new_arr) {
kfree(arr);
@@ -1041,8 +1044,9 @@ static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t size)
}
arr = new_arr;
- 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;
@@ -1074,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;
@@ -1090,11 +1094,12 @@ 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;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v2 3/3] bpf/verifier: Take advantage of full allocation sizes
2022-10-29 2:54 ` [PATCH bpf-next v2 3/3] bpf/verifier: Take advantage of full allocation sizes Kees Cook
@ 2022-10-31 21:53 ` Stanislav Fomichev
2022-11-01 5:23 ` Kees Cook
0 siblings, 1 reply; 12+ messages in thread
From: Stanislav Fomichev @ 2022-10-31 21:53 UTC (permalink / raw)
To: Kees Cook
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, linux-hardening
On Fri, Oct 28, 2022 at 7:54 PM Kees Cook <keescook@chromium.org> wrote:
>
> Since the full kmalloc bucket size is being explicitly allocated, pass
> back the resulting details to take advantage of the full size so that
> reallocation checking will be needed less frequently.
>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Martin KaFai Lau <martin.lau@linux.dev>
> Cc: Song Liu <song@kernel.org>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: KP Singh <kpsingh@kernel.org>
> Cc: Stanislav Fomichev <sdf@google.com>
> Cc: Hao Luo <haoluo@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: bpf@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> kernel/bpf/verifier.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1c040d27b8f6..e58b554e862b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1020,20 +1020,23 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
> 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)
> {
> size_t alloc_size;
> void *new_arr;
>
> - if (!new_n || old_n == new_n)
> + if (!new_n || !*new_n || old_n == *new_n)
> goto out;
>
> - alloc_size = kmalloc_size_roundup(size_mul(new_n, size));
> + alloc_size = kmalloc_size_roundup(size_mul(*new_n, size));
> new_arr = krealloc(arr, alloc_size, GFP_KERNEL);
> if (!new_arr) {
> kfree(arr);
> @@ -1041,8 +1044,9 @@ static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t size)
> }
> arr = new_arr;
>
> - 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;
[..]
> @@ -1074,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;
Patches 1 & 2 look good, but not sure about this part. We later do the
following in the same routine:
state->acquired_refs = n;
And acquire_reference_state() does "new_ofs = state->acquired_refs;" to append..
Which changes semantics a bit? It used to mean array size, now it
means array capacity.
Should we keep this part as is but add a shortcut to realloc_array
when ksize(ptr) == kmalloc_size_roundup(new size) -> reuse existing
array?
Or am I missing something? (haven't looked too deep)
> @@ -1090,11 +1094,12 @@ 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;
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH bpf-next v2 3/3] bpf/verifier: Take advantage of full allocation sizes
2022-10-31 21:53 ` Stanislav Fomichev
@ 2022-11-01 5:23 ` Kees Cook
0 siblings, 0 replies; 12+ messages in thread
From: Kees Cook @ 2022-11-01 5:23 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
KP Singh, Hao Luo, Jiri Olsa, bpf, linux-kernel, linux-hardening
On Mon, Oct 31, 2022 at 02:53:35PM -0700, Stanislav Fomichev wrote:
> On Fri, Oct 28, 2022 at 7:54 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > Since the full kmalloc bucket size is being explicitly allocated, pass
> > back the resulting details to take advantage of the full size so that
> > reallocation checking will be needed less frequently.
> >
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: John Fastabend <john.fastabend@gmail.com>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Martin KaFai Lau <martin.lau@linux.dev>
> > Cc: Song Liu <song@kernel.org>
> > Cc: Yonghong Song <yhs@fb.com>
> > Cc: KP Singh <kpsingh@kernel.org>
> > Cc: Stanislav Fomichev <sdf@google.com>
> > Cc: Hao Luo <haoluo@google.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: bpf@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > kernel/bpf/verifier.c | 27 ++++++++++++++++-----------
> > 1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 1c040d27b8f6..e58b554e862b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -1020,20 +1020,23 @@ static void *copy_array(void *dst, const void *src, size_t n, size_t size, gfp_t
> > 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)
> > {
> > size_t alloc_size;
> > void *new_arr;
> >
> > - if (!new_n || old_n == new_n)
> > + if (!new_n || !*new_n || old_n == *new_n)
> > goto out;
> >
> > - alloc_size = kmalloc_size_roundup(size_mul(new_n, size));
> > + alloc_size = kmalloc_size_roundup(size_mul(*new_n, size));
> > new_arr = krealloc(arr, alloc_size, GFP_KERNEL);
> > if (!new_arr) {
> > kfree(arr);
> > @@ -1041,8 +1044,9 @@ static void *realloc_array(void *arr, size_t old_n, size_t new_n, size_t size)
> > }
> > arr = new_arr;
> >
> > - 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;
>
> [..]
>
> > @@ -1074,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;
>
> Patches 1 & 2 look good, but not sure about this part. We later do the
> following in the same routine:
I'm totally fine leaving off #3. 1 is a bug fix, 2 is what I need to get
the ksize side-effect managed in bpf, and 3 was maybe an optimization.
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v2 0/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage
2022-10-29 2:54 [PATCH bpf-next v2 0/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage Kees Cook
` (2 preceding siblings ...)
2022-10-29 2:54 ` [PATCH bpf-next v2 3/3] bpf/verifier: Take advantage of full allocation sizes Kees Cook
@ 2022-11-01 13:40 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 12+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-11-01 13:40 UTC (permalink / raw)
To: Kees Cook
Cc: ast, daniel, john.fastabend, andrii, martin.lau, song, yhs,
kpsingh, sdf, haoluo, jolsa, linux-kernel, bpf, linux-hardening
Hello:
This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:
On Fri, 28 Oct 2022 19:54:29 -0700 you wrote:
> v2:
> - split up patch into logical changes
> - simplify copy_array, which can use ksize() directly
> v1: https://lore.kernel.org/all/20221018090550.never.834-kees@kernel.org/
>
> Hi,
>
> [...]
Here is the summary with links:
- [bpf-next,v2,1/3] bpf/verifier: Fix potential memory leak in array reallocation
https://git.kernel.org/bpf/bpf/c/42378a9ca553
- [bpf-next,v2,2/3] bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage
(no matching commit)
- [bpf-next,v2,3/3] bpf/verifier: Take advantage of full allocation sizes
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 12+ messages in thread