| Message ID | 4d7cc25e937403ac61ae61be06f998f27e631a65.1710522112.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [bpf-next,v3,1/2] bpf: Remove arch_unprotect_bpf_trampoline() | expand |
On 3/15/24 10:06, Christophe Leroy wrote: > arch_protect_bpf_trampoline() and alloc_new_pack() call > set_memory_rox() which can fail, leading to unprotected memory. > > Take into account return from set_memory_rox() function and add > __must_check flag to arch_protect_bpf_trampoline(). > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > Reviewed-by: Kees Cook <keescook@chromium.org> > --- > v3: > - Rebased and handled conflict in kernel/bpf/bpf_struct_ops.c > > v2: > - Move list_add_tail(&pack->list, &pack_list) at the end of alloc_new_pack() > - Split 2 lines that are reported longer than 80 chars by BPF patchwork's checkpatch report. > --- > arch/arm64/net/bpf_jit_comp.c | 3 ++- > arch/x86/net/bpf_jit_comp.c | 3 ++- > include/linux/bpf.h | 2 +- > kernel/bpf/bpf_struct_ops.c | 7 +++++-- > kernel/bpf/core.c | 29 ++++++++++++++++++++++------- > kernel/bpf/trampoline.c | 8 +++++--- > net/bpf/bpf_dummy_struct_ops.c | 4 +++- > 7 files changed, 40 insertions(+), 16 deletions(-) > > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c > index 132c8ffba109..bc16eb694657 100644 > --- a/arch/arm64/net/bpf_jit_comp.c > +++ b/arch/arm64/net/bpf_jit_comp.c > @@ -2176,8 +2176,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size) > bpf_prog_pack_free(image, size); > } > > -void arch_protect_bpf_trampoline(void *image, unsigned int size) > +int arch_protect_bpf_trampoline(void *image, unsigned int size) > { > + return 0; > } > > int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image, > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index 7a56d2d84512..4900b1ee019f 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -3004,8 +3004,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size) > bpf_prog_pack_free(image, size); > } > > -void arch_protect_bpf_trampoline(void *image, unsigned int size) > +int arch_protect_bpf_trampoline(void *image, unsigned int size) > { > + return 0; > } > > int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end, > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index d89bdefb42e2..17843e66a1d3 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1116,7 +1116,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > void *func_addr); > void *arch_alloc_bpf_trampoline(unsigned int size); > void arch_free_bpf_trampoline(void *image, unsigned int size); > -void arch_protect_bpf_trampoline(void *image, unsigned int size); > +int __must_check arch_protect_bpf_trampoline(void *image, unsigned int size); > int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags, > struct bpf_tramp_links *tlinks, void *func_addr); > > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c > index 43356faaa057..ca1d9b87c475 100644 > --- a/kernel/bpf/bpf_struct_ops.c > +++ b/kernel/bpf/bpf_struct_ops.c > @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, > if (err) > goto reset_unlock; > } > - for (i = 0; i < st_map->image_pages_cnt; i++) > - arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); > + for (i = 0; i < st_map->image_pages_cnt && !err; i++) > + err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); > + > + if (err) nit: Can it be more specific? I mean to check err < 0, so we can reason that this function never returns a positive value other than 0. > + goto reset_unlock; > > if (st_map->map.map_flags & BPF_F_LINK) { > err = 0; > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index 63f100def31b..1e761c3c66db 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -908,23 +908,31 @@ static LIST_HEAD(pack_list); > static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_insns) > { > struct bpf_prog_pack *pack; > + int err; > > pack = kzalloc(struct_size(pack, bitmap, BITS_TO_LONGS(BPF_PROG_CHUNK_COUNT)), > GFP_KERNEL); > if (!pack) > return NULL; > pack->ptr = bpf_jit_alloc_exec(BPF_PROG_PACK_SIZE); > - if (!pack->ptr) { > - kfree(pack); > - return NULL; > - } > + if (!pack->ptr) > + goto out; > bpf_fill_ill_insns(pack->ptr, BPF_PROG_PACK_SIZE); > bitmap_zero(pack->bitmap, BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE); > - list_add_tail(&pack->list, &pack_list); > > set_vm_flush_reset_perms(pack->ptr); > - set_memory_rox((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE); > + err = set_memory_rox((unsigned long)pack->ptr, > + BPF_PROG_PACK_SIZE / PAGE_SIZE); > + if (err) > + goto out_free; > + list_add_tail(&pack->list, &pack_list); > return pack; > + > +out_free: > + bpf_jit_free_exec(pack->ptr); > +out: > + kfree(pack); > + return NULL; > } > > void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) > @@ -939,9 +947,16 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) > size = round_up(size, PAGE_SIZE); > ptr = bpf_jit_alloc_exec(size); > if (ptr) { > + int err; > + > bpf_fill_ill_insns(ptr, size); > set_vm_flush_reset_perms(ptr); > - set_memory_rox((unsigned long)ptr, size / PAGE_SIZE); > + err = set_memory_rox((unsigned long)ptr, > + size / PAGE_SIZE); > + if (err) { > + bpf_jit_free_exec(ptr); > + ptr = NULL; > + } > } > goto out; > } > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > index 04fd1abd3661..cc50607f8d8c 100644 > --- a/kernel/bpf/trampoline.c > +++ b/kernel/bpf/trampoline.c > @@ -456,7 +456,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut > if (err < 0) > goto out_free; Same here > > - arch_protect_bpf_trampoline(im->image, im->size); > + err = arch_protect_bpf_trampoline(im->image, im->size); > + if (err) > + goto out_free; > > WARN_ON(tr->cur_image && total == 0); > if (tr->cur_image) > @@ -1072,10 +1074,10 @@ void __weak arch_free_bpf_trampoline(void *image, unsigned int size) > bpf_jit_free_exec(image); > } > > -void __weak arch_protect_bpf_trampoline(void *image, unsigned int size) > +int __weak arch_protect_bpf_trampoline(void *image, unsigned int size) > { > WARN_ON_ONCE(size > PAGE_SIZE); > - set_memory_rox((long)image, 1); > + return set_memory_rox((long)image, 1); > } > > int __weak arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags, > diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c > index de33dc1b0daa..25b75844891a 100644 > --- a/net/bpf/bpf_dummy_struct_ops.c > +++ b/net/bpf/bpf_dummy_struct_ops.c > @@ -133,7 +133,9 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr, > if (err < 0) > goto out; > > - arch_protect_bpf_trampoline(image, PAGE_SIZE); > + err = arch_protect_bpf_trampoline(image, PAGE_SIZE); > + if (err) Same here. > + goto out; > prog_ret = dummy_ops_call_op(image, args); > > err = dummy_ops_copy_args(args);
On 3/15/24 11:11 AM, Kui-Feng Lee wrote: >> --- a/kernel/bpf/bpf_struct_ops.c >> +++ b/kernel/bpf/bpf_struct_ops.c >> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map >> *map, void *key, >> if (err) >> goto reset_unlock; >> } >> - for (i = 0; i < st_map->image_pages_cnt; i++) >> - arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); >> + for (i = 0; i < st_map->image_pages_cnt && !err; i++) >> + err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); >> + >> + if (err) > > nit: Can it be more specific? I mean to check err < 0, so we can reason > that this function never returns a positive value other than 0. I think "if (err)" is fine. It is pretty common in other places of the kernel. Checking "(err < 0)" may actually mean the return value could be positive. At least it is how bpf_struct_ops.c is using "(err < 0)". [ An unrelated side note is another (err < 0) check in bpf_struct_ops.c could have been changed after the recent changes in bpf_struct_ops_prepare_trampoline which no longer return +val ].
On 3/15/24 10:06 AM, Christophe Leroy wrote: > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c > index 43356faaa057..ca1d9b87c475 100644 > --- a/kernel/bpf/bpf_struct_ops.c > +++ b/kernel/bpf/bpf_struct_ops.c > @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, > if (err) > goto reset_unlock; > } > - for (i = 0; i < st_map->image_pages_cnt; i++) > - arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); > + for (i = 0; i < st_map->image_pages_cnt && !err; i++) > + err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); > + > + if (err) > + goto reset_unlock; This part does not look right. The "if (err)" check should be inside the for loop. pw-bot: cr
On 3/15/24 1:55 PM, Martin KaFai Lau wrote: > On 3/15/24 10:06 AM, Christophe Leroy wrote: >> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c >> index 43356faaa057..ca1d9b87c475 100644 >> --- a/kernel/bpf/bpf_struct_ops.c >> +++ b/kernel/bpf/bpf_struct_ops.c >> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map >> *map, void *key, >> if (err) >> goto reset_unlock; >> } >> - for (i = 0; i < st_map->image_pages_cnt; i++) >> - arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); >> + for (i = 0; i < st_map->image_pages_cnt && !err; i++) >> + err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); >> + >> + if (err) >> + goto reset_unlock; > > This part does not look right. The "if (err)" check should be inside the for loop. ah. Please ignore. missed the "!err" in the for loop.
On 3/15/24 2:11 PM, Martin KaFai Lau wrote: > On 3/15/24 1:55 PM, Martin KaFai Lau wrote: >> On 3/15/24 10:06 AM, Christophe Leroy wrote: >>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c >>> index 43356faaa057..ca1d9b87c475 100644 >>> --- a/kernel/bpf/bpf_struct_ops.c >>> +++ b/kernel/bpf/bpf_struct_ops.c >>> @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct >>> bpf_map *map, void *key, >>> if (err) >>> goto reset_unlock; >>> } >>> - for (i = 0; i < st_map->image_pages_cnt; i++) >>> - arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); >>> + for (i = 0; i < st_map->image_pages_cnt && !err; i++) I was about to apply but I still think checking "&& !err" is not right given how "err" is used in the earlier code of this function. The err may not be 0 in the first iteration of this for loop. Take a look at the "if (err > 0)" check in the "for_each_member(i, t, member)" loop above. >>> + err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); >>> + >>> + if (err) >>> + goto reset_unlock; >> >> This part does not look right. The "if (err)" check should be inside the for >> loop. Instead of adding an extra "err = 0;" before the for loop. It is better to move this "if (err) goto reset_unlock;" into the for loop and remove the "&& !err" test above. > > ah. Please ignore. missed the "!err" in the for loop.
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c index 132c8ffba109..bc16eb694657 100644 --- a/arch/arm64/net/bpf_jit_comp.c +++ b/arch/arm64/net/bpf_jit_comp.c @@ -2176,8 +2176,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size) bpf_prog_pack_free(image, size); } -void arch_protect_bpf_trampoline(void *image, unsigned int size) +int arch_protect_bpf_trampoline(void *image, unsigned int size) { + return 0; } int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image, diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index 7a56d2d84512..4900b1ee019f 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -3004,8 +3004,9 @@ void arch_free_bpf_trampoline(void *image, unsigned int size) bpf_prog_pack_free(image, size); } -void arch_protect_bpf_trampoline(void *image, unsigned int size) +int arch_protect_bpf_trampoline(void *image, unsigned int size) { + return 0; } int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end, diff --git a/include/linux/bpf.h b/include/linux/bpf.h index d89bdefb42e2..17843e66a1d3 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1116,7 +1116,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i void *func_addr); void *arch_alloc_bpf_trampoline(unsigned int size); void arch_free_bpf_trampoline(void *image, unsigned int size); -void arch_protect_bpf_trampoline(void *image, unsigned int size); +int __must_check arch_protect_bpf_trampoline(void *image, unsigned int size); int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags, struct bpf_tramp_links *tlinks, void *func_addr); diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c index 43356faaa057..ca1d9b87c475 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -742,8 +742,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, if (err) goto reset_unlock; } - for (i = 0; i < st_map->image_pages_cnt; i++) - arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); + for (i = 0; i < st_map->image_pages_cnt && !err; i++) + err = arch_protect_bpf_trampoline(st_map->image_pages[i], PAGE_SIZE); + + if (err) + goto reset_unlock; if (st_map->map.map_flags & BPF_F_LINK) { err = 0; diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 63f100def31b..1e761c3c66db 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -908,23 +908,31 @@ static LIST_HEAD(pack_list); static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_insns) { struct bpf_prog_pack *pack; + int err; pack = kzalloc(struct_size(pack, bitmap, BITS_TO_LONGS(BPF_PROG_CHUNK_COUNT)), GFP_KERNEL); if (!pack) return NULL; pack->ptr = bpf_jit_alloc_exec(BPF_PROG_PACK_SIZE); - if (!pack->ptr) { - kfree(pack); - return NULL; - } + if (!pack->ptr) + goto out; bpf_fill_ill_insns(pack->ptr, BPF_PROG_PACK_SIZE); bitmap_zero(pack->bitmap, BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE); - list_add_tail(&pack->list, &pack_list); set_vm_flush_reset_perms(pack->ptr); - set_memory_rox((unsigned long)pack->ptr, BPF_PROG_PACK_SIZE / PAGE_SIZE); + err = set_memory_rox((unsigned long)pack->ptr, + BPF_PROG_PACK_SIZE / PAGE_SIZE); + if (err) + goto out_free; + list_add_tail(&pack->list, &pack_list); return pack; + +out_free: + bpf_jit_free_exec(pack->ptr); +out: + kfree(pack); + return NULL; } void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) @@ -939,9 +947,16 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns) size = round_up(size, PAGE_SIZE); ptr = bpf_jit_alloc_exec(size); if (ptr) { + int err; + bpf_fill_ill_insns(ptr, size); set_vm_flush_reset_perms(ptr); - set_memory_rox((unsigned long)ptr, size / PAGE_SIZE); + err = set_memory_rox((unsigned long)ptr, + size / PAGE_SIZE); + if (err) { + bpf_jit_free_exec(ptr); + ptr = NULL; + } } goto out; } diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index 04fd1abd3661..cc50607f8d8c 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -456,7 +456,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut if (err < 0) goto out_free; - arch_protect_bpf_trampoline(im->image, im->size); + err = arch_protect_bpf_trampoline(im->image, im->size); + if (err) + goto out_free; WARN_ON(tr->cur_image && total == 0); if (tr->cur_image) @@ -1072,10 +1074,10 @@ void __weak arch_free_bpf_trampoline(void *image, unsigned int size) bpf_jit_free_exec(image); } -void __weak arch_protect_bpf_trampoline(void *image, unsigned int size) +int __weak arch_protect_bpf_trampoline(void *image, unsigned int size) { WARN_ON_ONCE(size > PAGE_SIZE); - set_memory_rox((long)image, 1); + return set_memory_rox((long)image, 1); } int __weak arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags, diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c index de33dc1b0daa..25b75844891a 100644 --- a/net/bpf/bpf_dummy_struct_ops.c +++ b/net/bpf/bpf_dummy_struct_ops.c @@ -133,7 +133,9 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr, if (err < 0) goto out; - arch_protect_bpf_trampoline(image, PAGE_SIZE); + err = arch_protect_bpf_trampoline(image, PAGE_SIZE); + if (err) + goto out; prog_ret = dummy_ops_call_op(image, args); err = dummy_ops_copy_args(args);