| Message ID | 20221029025433.2533810-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 42378a9ca55347102bbf86708776061d8fe3ece2 |
| Headers | show |
| Series | bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage | expand |
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 >
[ +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 >>
On Tue, 1 Nov 2022, at 13:46, Daniel Borkmann wrote:
> [ +Lorenz ]
Thank you Kees and Daniel for fixing this!
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);
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(-)