| Message ID | 20221129042644.231816-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf, test_run: fix alignment problem in bpf_test_init() | expand |
Hi Zhengchao,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Zhengchao-Shao/bpf-test_run-fix-alignment-problem-in-bpf_test_init/20221129-122226
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20221129042644.231816-1-shaozhengchao%40huawei.com
patch subject: [PATCH bpf-next] bpf, test_run: fix alignment problem in bpf_test_init()
config: i386-randconfig-m021-20221205
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
smatch warnings:
net/bpf/test_run.c:786 bpf_test_init() warn: inconsistent indenting
vim +786 net/bpf/test_run.c
764
765 static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
766 u32 size, u32 headroom, u32 tailroom)
767 {
768 void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
769 unsigned int true_size;
770 void *true_data;
771 void *data;
772
773 if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)
774 return ERR_PTR(-EINVAL);
775
776 if (user_size > size)
777 return ERR_PTR(-EMSGSIZE);
778
779 data = kzalloc(size + headroom + tailroom, GFP_USER);
780 if (!data)
781 return ERR_PTR(-ENOMEM);
782
783 true_size = ksize(data);
784 if (size + headroom + tailroom < true_size) {
785 true_data = krealloc(data, true_size, GFP_USER | __GFP_ZERO);
> 786 if (!true_data)
787 return ERR_PTR(-ENOMEM);
788 data = true_data;
789 }
790
791 if (copy_from_user(data + headroom, data_in, user_size)) {
792 kfree(data);
793 return ERR_PTR(-EFAULT);
794 }
795
796 return data;
797 }
798
Zhengchao Shao wrote: > The problem reported by syz is as follows: > BUG: KASAN: slab-out-of-bounds in __build_skb_around+0x230/0x330 > Write of size 32 at addr ffff88807ec6b2c0 by task bpf_repo/6711 > Call Trace: > <TASK> > dump_stack_lvl+0x8e/0xd1 > print_report+0x155/0x454 > kasan_report+0xba/0x1f0 > kasan_check_range+0x35/0x1b0 > memset+0x20/0x40 > __build_skb_around+0x230/0x330 > build_skb+0x4c/0x260 > bpf_prog_test_run_skb+0x2fc/0x1ce0 > __sys_bpf+0x1798/0x4b60 > __x64_sys_bpf+0x75/0xb0 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > </TASK> > > Allocated by task 6711: > kasan_save_stack+0x1e/0x40 > kasan_set_track+0x21/0x30 > __kasan_kmalloc+0xa1/0xb0 > __kmalloc+0x4e/0xb0 > bpf_test_init.isra.0+0x77/0x100 > bpf_prog_test_run_skb+0x219/0x1ce0 > __sys_bpf+0x1798/0x4b60 > __x64_sys_bpf+0x75/0xb0 > do_syscall_64+0x35/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > The process is as follows: > bpf_prog_test_run_skb() > bpf_test_init() > data = kzalloc() //The length of input is 576. > //The actual allocated memory > //size is 1024. > build_skb() > __build_skb_around() > size = ksize(data)//size = 1024 > size -= SKB_DATA_ALIGN( > sizeof(struct skb_shared_info)); > //size = 704 > skb_set_end_offset(skb, size); > shinfo = skb_shinfo(skb);//shinfo = data + 704 > memset(shinfo...) //Write out of bounds > > In bpf_test_init(), the accessible space allocated to data is 576 bytes, > and the memory allocated to data is 1024 bytes. In __build_skb_around(), > shinfo indicates the offset of 704 bytes of data, which triggers the issue > of writing out of bounds. > > Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") > Reported-by: syzbot+fda18eaa8c12534ccb3b@syzkaller.appspotmail.com > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > net/bpf/test_run.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index fcb3e6c5e03c..fbd5337b8f68 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -766,6 +766,8 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, > u32 size, u32 headroom, u32 tailroom) > { > void __user *data_in = u64_to_user_ptr(kattr->test.data_in); > + unsigned int true_size; > + void *true_data; > void *data; > > if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom) > @@ -779,6 +781,14 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, > if (!data) > return ERR_PTR(-ENOMEM); > > + true_size = ksize(data); > + if (size + headroom + tailroom < true_size) { > + true_data = krealloc(data, true_size, GFP_USER | __GFP_ZERO); This comes from a kzalloc, should we zero realloc'd memory as well? > + if (!true_data) > + return ERR_PTR(-ENOMEM); I think its worth fixing the extra tab here. > + data = true_data; > + } > + > if (copy_from_user(data + headroom, data_in, user_size)) { > kfree(data); > return ERR_PTR(-EFAULT); > -- > 2.17.1 >
On 2022/12/7 3:58, John Fastabend wrote: > Zhengchao Shao wrote: >> The problem reported by syz is as follows: >> BUG: KASAN: slab-out-of-bounds in __build_skb_around+0x230/0x330 >> Write of size 32 at addr ffff88807ec6b2c0 by task bpf_repo/6711 >> Call Trace: >> <TASK> >> dump_stack_lvl+0x8e/0xd1 >> print_report+0x155/0x454 >> kasan_report+0xba/0x1f0 >> kasan_check_range+0x35/0x1b0 >> memset+0x20/0x40 >> __build_skb_around+0x230/0x330 >> build_skb+0x4c/0x260 >> bpf_prog_test_run_skb+0x2fc/0x1ce0 >> __sys_bpf+0x1798/0x4b60 >> __x64_sys_bpf+0x75/0xb0 >> do_syscall_64+0x35/0x80 >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> </TASK> >> >> Allocated by task 6711: >> kasan_save_stack+0x1e/0x40 >> kasan_set_track+0x21/0x30 >> __kasan_kmalloc+0xa1/0xb0 >> __kmalloc+0x4e/0xb0 >> bpf_test_init.isra.0+0x77/0x100 >> bpf_prog_test_run_skb+0x219/0x1ce0 >> __sys_bpf+0x1798/0x4b60 >> __x64_sys_bpf+0x75/0xb0 >> do_syscall_64+0x35/0x80 >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> >> The process is as follows: >> bpf_prog_test_run_skb() >> bpf_test_init() >> data = kzalloc() //The length of input is 576. >> //The actual allocated memory >> //size is 1024. >> build_skb() >> __build_skb_around() >> size = ksize(data)//size = 1024 >> size -= SKB_DATA_ALIGN( >> sizeof(struct skb_shared_info)); >> //size = 704 >> skb_set_end_offset(skb, size); >> shinfo = skb_shinfo(skb);//shinfo = data + 704 >> memset(shinfo...) //Write out of bounds >> >> In bpf_test_init(), the accessible space allocated to data is 576 bytes, >> and the memory allocated to data is 1024 bytes. In __build_skb_around(), >> shinfo indicates the offset of 704 bytes of data, which triggers the issue >> of writing out of bounds. >> >> Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") >> Reported-by: syzbot+fda18eaa8c12534ccb3b@syzkaller.appspotmail.com >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- >> net/bpf/test_run.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c >> index fcb3e6c5e03c..fbd5337b8f68 100644 >> --- a/net/bpf/test_run.c >> +++ b/net/bpf/test_run.c >> @@ -766,6 +766,8 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, >> u32 size, u32 headroom, u32 tailroom) >> { >> void __user *data_in = u64_to_user_ptr(kattr->test.data_in); >> + unsigned int true_size; >> + void *true_data; >> void *data; >> >> if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom) >> @@ -779,6 +781,14 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, >> if (!data) >> return ERR_PTR(-ENOMEM); >> >> + true_size = ksize(data); >> + if (size + headroom + tailroom < true_size) { >> + true_data = krealloc(data, true_size, GFP_USER | __GFP_ZERO); > > This comes from a kzalloc, should we zero realloc'd memory as well? > >> + if (!true_data) >> + return ERR_PTR(-ENOMEM); > > I think its worth fixing the extra tab here. > Hi John: Thank you for your review. Your suggestion looks good to me. And I found Kees Cook also focus on this issue. https://patchwork.kernel.org/project/netdevbpf/patch/20221206231659.never.929-kees@kernel.org/ Perhaps his solution will be more common? Zhengchao Shao >> + data = true_data; >> + } >> + >> if (copy_from_user(data + headroom, data_in, user_size)) { >> kfree(data); >> return ERR_PTR(-EFAULT); >> -- >> 2.17.1 >>
shaozhengchao wrote: > > > On 2022/12/7 3:58, John Fastabend wrote: > > Zhengchao Shao wrote: > >> The problem reported by syz is as follows: > >> BUG: KASAN: slab-out-of-bounds in __build_skb_around+0x230/0x330 > >> Write of size 32 at addr ffff88807ec6b2c0 by task bpf_repo/6711 > >> Call Trace: > >> <TASK> > >> dump_stack_lvl+0x8e/0xd1 > >> print_report+0x155/0x454 > >> kasan_report+0xba/0x1f0 > >> kasan_check_range+0x35/0x1b0 > >> memset+0x20/0x40 > >> __build_skb_around+0x230/0x330 > >> build_skb+0x4c/0x260 > >> bpf_prog_test_run_skb+0x2fc/0x1ce0 > >> __sys_bpf+0x1798/0x4b60 > >> __x64_sys_bpf+0x75/0xb0 > >> do_syscall_64+0x35/0x80 > >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 > >> </TASK> > >> > >> Allocated by task 6711: > >> kasan_save_stack+0x1e/0x40 > >> kasan_set_track+0x21/0x30 > >> __kasan_kmalloc+0xa1/0xb0 > >> __kmalloc+0x4e/0xb0 > >> bpf_test_init.isra.0+0x77/0x100 > >> bpf_prog_test_run_skb+0x219/0x1ce0 > >> __sys_bpf+0x1798/0x4b60 > >> __x64_sys_bpf+0x75/0xb0 > >> do_syscall_64+0x35/0x80 > >> entry_SYSCALL_64_after_hwframe+0x46/0xb0 > >> > >> The process is as follows: > >> bpf_prog_test_run_skb() > >> bpf_test_init() > >> data = kzalloc() //The length of input is 576. > >> //The actual allocated memory > >> //size is 1024. > >> build_skb() > >> __build_skb_around() > >> size = ksize(data)//size = 1024 > >> size -= SKB_DATA_ALIGN( > >> sizeof(struct skb_shared_info)); > >> //size = 704 > >> skb_set_end_offset(skb, size); > >> shinfo = skb_shinfo(skb);//shinfo = data + 704 > >> memset(shinfo...) //Write out of bounds > >> > >> In bpf_test_init(), the accessible space allocated to data is 576 bytes, > >> and the memory allocated to data is 1024 bytes. In __build_skb_around(), > >> shinfo indicates the offset of 704 bytes of data, which triggers the issue > >> of writing out of bounds. > >> > >> Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") > >> Reported-by: syzbot+fda18eaa8c12534ccb3b@syzkaller.appspotmail.com > >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > >> --- > >> net/bpf/test_run.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > >> index fcb3e6c5e03c..fbd5337b8f68 100644 > >> --- a/net/bpf/test_run.c > >> +++ b/net/bpf/test_run.c > >> @@ -766,6 +766,8 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, > >> u32 size, u32 headroom, u32 tailroom) > >> { > >> void __user *data_in = u64_to_user_ptr(kattr->test.data_in); > >> + unsigned int true_size; > >> + void *true_data; > >> void *data; > >> > >> if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom) > >> @@ -779,6 +781,14 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, > >> if (!data) > >> return ERR_PTR(-ENOMEM); > >> > >> + true_size = ksize(data); > >> + if (size + headroom + tailroom < true_size) { > >> + true_data = krealloc(data, true_size, GFP_USER | __GFP_ZERO); > > > > This comes from a kzalloc, should we zero realloc'd memory as well? > > > >> + if (!true_data) > >> + return ERR_PTR(-ENOMEM); > > > > I think its worth fixing the extra tab here. > > > > Hi John: > Thank you for your review. Your suggestion looks good to me. And I > found Kees Cook also focus on this issue. > https://patchwork.kernel.org/project/netdevbpf/patch/20221206231659.never.929-kees@kernel.org/ > Perhaps his solution will be more common? Maybe, seems ksize should not be used either.
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index fcb3e6c5e03c..fbd5337b8f68 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -766,6 +766,8 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, u32 size, u32 headroom, u32 tailroom) { void __user *data_in = u64_to_user_ptr(kattr->test.data_in); + unsigned int true_size; + void *true_data; void *data; if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom) @@ -779,6 +781,14 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, if (!data) return ERR_PTR(-ENOMEM); + true_size = ksize(data); + if (size + headroom + tailroom < true_size) { + true_data = krealloc(data, true_size, GFP_USER | __GFP_ZERO); + if (!true_data) + return ERR_PTR(-ENOMEM); + data = true_data; + } + if (copy_from_user(data + headroom, data_in, user_size)) { kfree(data); return ERR_PTR(-EFAULT);
The problem reported by syz is as follows: BUG: KASAN: slab-out-of-bounds in __build_skb_around+0x230/0x330 Write of size 32 at addr ffff88807ec6b2c0 by task bpf_repo/6711 Call Trace: <TASK> dump_stack_lvl+0x8e/0xd1 print_report+0x155/0x454 kasan_report+0xba/0x1f0 kasan_check_range+0x35/0x1b0 memset+0x20/0x40 __build_skb_around+0x230/0x330 build_skb+0x4c/0x260 bpf_prog_test_run_skb+0x2fc/0x1ce0 __sys_bpf+0x1798/0x4b60 __x64_sys_bpf+0x75/0xb0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 </TASK> Allocated by task 6711: kasan_save_stack+0x1e/0x40 kasan_set_track+0x21/0x30 __kasan_kmalloc+0xa1/0xb0 __kmalloc+0x4e/0xb0 bpf_test_init.isra.0+0x77/0x100 bpf_prog_test_run_skb+0x219/0x1ce0 __sys_bpf+0x1798/0x4b60 __x64_sys_bpf+0x75/0xb0 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 The process is as follows: bpf_prog_test_run_skb() bpf_test_init() data = kzalloc() //The length of input is 576. //The actual allocated memory //size is 1024. build_skb() __build_skb_around() size = ksize(data)//size = 1024 size -= SKB_DATA_ALIGN( sizeof(struct skb_shared_info)); //size = 704 skb_set_end_offset(skb, size); shinfo = skb_shinfo(skb);//shinfo = data + 704 memset(shinfo...) //Write out of bounds In bpf_test_init(), the accessible space allocated to data is 576 bytes, and the memory allocated to data is 1024 bytes. In __build_skb_around(), shinfo indicates the offset of 704 bytes of data, which triggers the issue of writing out of bounds. Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") Reported-by: syzbot+fda18eaa8c12534ccb3b@syzkaller.appspotmail.com Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- net/bpf/test_run.c | 10 ++++++++++ 1 file changed, 10 insertions(+)