v2:
- split up patch into logical changes
- simplify copy_array, which can use ksize() directly
v1: https://lore.kernel.org/all/[email protected]/
Hi,
Here's the next version of removing the BPF verifier's dependency on the
side-effects of ksize(), so we can remove the special handling needed
for KASAN, UBSAN_BOUNDS, nor FORTIFY_SOURCE in ksize().
Thanks,
-Kees
Kees Cook (3):
bpf/verifier: Fix potential memory leak in array reallocation
bpf/verifier: Use kmalloc_size_roundup() to match ksize() usage
bpf/verifier: Take advantage of full allocation sizes
kernel/bpf/verifier.c | 44 ++++++++++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 15 deletions(-)
--
2.34.1
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 <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: John Fastabend <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Martin KaFai Lau <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Yonghong Song <[email protected]>
Cc: KP Singh <[email protected]>
Cc: Stanislav Fomichev <[email protected]>
Cc: Hao Luo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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
On Fri, Oct 28, 2022 at 7:55 PM Kees Cook <[email protected]> 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 <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: John Fastabend <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Martin KaFai Lau <[email protected]>
> Cc: Song Liu <[email protected]>
> Cc: Yonghong Song <[email protected]>
> Cc: KP Singh <[email protected]>
> Cc: Stanislav Fomichev <[email protected]>
> Cc: Hao Luo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Bill Wendling <[email protected]>
> ---
> 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
>
Hello:
This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <[email protected]>:
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/[email protected]/
>
> 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
[ +Lorenz ]
On 10/31/22 9:16 PM, Bill Wendling wrote:
> On Fri, Oct 28, 2022 at 7:55 PM Kees Cook <[email protected]> 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 <[email protected]>
>> Cc: Daniel Borkmann <[email protected]>
>> Cc: John Fastabend <[email protected]>
>> Cc: Andrii Nakryiko <[email protected]>
>> Cc: Martin KaFai Lau <[email protected]>
>> Cc: Song Liu <[email protected]>
>> Cc: Yonghong Song <[email protected]>
>> Cc: KP Singh <[email protected]>
>> Cc: Stanislav Fomichev <[email protected]>
>> Cc: Hao Luo <[email protected]>
>> Cc: Jiri Olsa <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Kees Cook <[email protected]>
>
> Reviewed-by: Bill Wendling <[email protected]>
>
>> ---
>> 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/[email protected]/
>> 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!