| Message ID | 1726037521-18232-1-git-send-email-kongln9170@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix a bug in ebpf verifier | expand |
On Wed, 2024-09-11 at 14:52 +0800, lonial con wrote: > In find_equal_scalars(), it should not copy the reg->subreg_def, otherwise a bug will occur when the program flag has BPF_F_TEST_RND_HI32. > > Reported-by: Lonial Con <kongln9170@gmail.com> > Signed-off-by: Lonial Con <kongln9170@gmail.com> > --- Hello, could you please write a selftest for this fix? (please let me know if you need some intro on BPF selftests). [...]
Hi, I have never written a selftest before. I wrote a simple POC to demonstrate this bug. This POC can crash the Linux kernel 6.6.50. I think the ebpf code in the POC will be helpful for writing a selftest. Thanks, Lonial Con Eduard Zingerman <eddyz87@gmail.com> 于2024年9月11日周三 22:54写道： > On Wed, 2024-09-11 at 14:52 +0800, lonial con wrote: > > In find_equal_scalars(), it should not copy the reg->subreg_def, > otherwise a bug will occur when the program flag has BPF_F_TEST_RND_HI32. > > > > Reported-by: Lonial Con <kongln9170@gmail.com> > > Signed-off-by: Lonial Con <kongln9170@gmail.com> > > --- > > Hello, > > could you please write a selftest for this fix? > (please let me know if you need some intro on BPF selftests). > > [...] > >
On Thu, 2024-09-12 at 10:53 +0800, lonial con wrote: > I have never written a selftest before. I wrote a simple POC to > demonstrate this bug. This POC can crash the Linux kernel 6.6.50. I > think the ebpf code in the POC will be helpful for writing a > selftest. Well all depends on how familiar you want to get with selftests infrastructure :) Here is a promised intro. If you don't want to bother, please let me know, I can write the selftest. If you do want to bother, feel free to ask any questions. *** Please find a minimal recipe allowing to compile and run selftests in a chroot at the bottom of this email. You would probably want to adjust it, e.g. setup a user matching your local user inside chroot and do a bind mount for sources directory etc. After setting up the environment you will have to write the test. BPF selftests reside in the following directory: - tools/testing/selftests/bpf/ Nowadays we mostly add selftests to test_progs executable and use bpftool skeletons / libbpf to simplify maps and programs creation. The files located under prog_tests/ directory are compiled as host programs, the files located under progs/ are compiled as BPF programs (and libbpf skeletons are generated for these programs). Skeletons generated for files from progs/ are used in tests declared in prog_tests/. Your POC structure: - sets up a few maps - sets up data for ringbuf - loads and runs BPF program You can look at a selftests that have similar structure, e.g.: - tools/testing/selftests/bpf/prog_tests/ringbuf.c - tools/testing/selftests/bpf/progs/test_ringbuf.c Interesting parts of the 'prog_tests/ringbuf.c': // this includes skeleton generated by bpftool #include "test_ringbuf.lskel.h" static void ringbuf_subtest(void) { ... // use generated methods to setup maps and programs skel = test_ringbuf_lskel__open(); ... err = test_ringbuf_lskel__load(skel); // you can do bpf_prog_run here as well } // build system automatically wires up functions // void test_*(void) as entry points for tests // executed by test_progs binary void test_ringbuf(void) { // needed for tests filtering, e.g. -t option for test_progs if (test__start_subtest("ringbuf")) ringbuf_subtest(); ... } *** chroot selftests build/run recipe follows *** # First, setup the bullseye chroot sudo /usr/sbin/debootstrap --variant=buildd --arch=amd64 bullseye bullseye-chroot/ http://deb.debian.org/debian # provide {dev,proc} for chroot sudo mount --rbind /dev ./bullseye-chroot/dev/ sudo mount --make-rslave ./bullseye-chroot/dev/ sudo mount -t proc proc ./bullseye-chroot/proc/ # enter chroot sudo chroot ./bullseye-chroot # Install build tools apt install build-essential bc flex bison git libelf-dev libssl-dev \ docutils-common rsync curl zstd qemu-system-x86 sudo cmake \ libdw-dev lsb-release wget software-properties-common gnupg e2fsprogs # Install fresh clang-18 snapshot, the llvm.sh sets up some repos curl https://apt.llvm.org/llvm.sh --output /tmp/llvm.sh bash /tmp/llvm.sh 18 apt install clang-tools-18 ln -s /usr/bin/clang-18 /usr/bin/clang ln -s /usr/bin/llvm-strip-18 /usr/bin/llvm-strip # that would be /root inside chroot cd $HOME # Get and compile pahole, use instructions from: # https://git.kernel.org/pub/scm/devel/pahole/pahole.git/about/ git clone https://git.kernel.org/pub/scm/devel/pahole/pahole.git cd pahole git submodule update --init --recursive mkdir build cd build cmake -D__LIB=lib .. make -j # make it available system-wide ln -s $(realpath pahole) /usr/local/bin/ cd $HOME git clone --depth=1 https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git # Run vmtests, this should download rootfs, build kernel and tests, run test_verifier # vmtest.sh would ask for root password to mount rootfs image cd bpf-next/tools/testing/selftests/bpf ./vmtest.sh -- ./test_verifier # And now run test_progs ./vmtest.sh -- ./test_progs # One can filter tests too ./vmtest.sh -- ./test_progs -t ringbuf
Hi, I tried to build this environment, but it seems that it needs kvm support. For me, it is very troublesome to prepare a kvm environment. So could you please write this selftest? Thanks, Lonial Con lonial con <kongln9170@gmail.com> 于2024年9月12日周四 21:31写道： > > Hi, > > I tried to build this environment, but it seems that it needs kvm support. For me, it is very troublesome to prepare a kvm environment. So could you please write this selftest? > > Thanks, > Lonial Con > > Eduard Zingerman <eddyz87@gmail.com> 于2024年9月12日周四 12:31写道： >> >> On Thu, 2024-09-12 at 10:53 +0800, lonial con wrote: >> >> > I have never written a selftest before. I wrote a simple POC to >> > demonstrate this bug. This POC can crash the Linux kernel 6.6.50. I >> > think the ebpf code in the POC will be helpful for writing a >> > selftest. >> >> Well all depends on how familiar you want to get with selftests >> infrastructure :) Here is a promised intro. If you don't want to >> bother, please let me know, I can write the selftest. >> If you do want to bother, feel free to ask any questions. >> >> *** >> >> Please find a minimal recipe allowing to compile and run selftests in >> a chroot at the bottom of this email. You would probably want to >> adjust it, e.g. setup a user matching your local user inside chroot >> and do a bind mount for sources directory etc. >> >> After setting up the environment you will have to write the test. >> >> BPF selftests reside in the following directory: >> - tools/testing/selftests/bpf/ >> >> Nowadays we mostly add selftests to test_progs executable and use >> bpftool skeletons / libbpf to simplify maps and programs creation. >> >> The files located under prog_tests/ directory are compiled as host >> programs, the files located under progs/ are compiled as BPF programs >> (and libbpf skeletons are generated for these programs). >> Skeletons generated for files from progs/ are used in tests declared >> in prog_tests/. >> >> Your POC structure: >> - sets up a few maps >> - sets up data for ringbuf >> - loads and runs BPF program >> >> You can look at a selftests that have similar structure, e.g.: >> - tools/testing/selftests/bpf/prog_tests/ringbuf.c >> - tools/testing/selftests/bpf/progs/test_ringbuf.c >> >> Interesting parts of the 'prog_tests/ringbuf.c': >> >> // this includes skeleton generated by bpftool >> #include "test_ringbuf.lskel.h" >> >> static void ringbuf_subtest(void) >> { >> ... >> // use generated methods to setup maps and programs >> skel = test_ringbuf_lskel__open(); >> ... >> err = test_ringbuf_lskel__load(skel); >> >> // you can do bpf_prog_run here as well >> } >> >> // build system automatically wires up functions >> // void test_*(void) as entry points for tests >> // executed by test_progs binary >> void test_ringbuf(void) >> { >> // needed for tests filtering, e.g. -t option for test_progs >> if (test__start_subtest("ringbuf")) >> ringbuf_subtest(); >> ... >> } >> >> >> *** chroot selftests build/run recipe follows *** >> >> # First, setup the bullseye chroot >> sudo /usr/sbin/debootstrap --variant=buildd --arch=amd64 bullseye bullseye-chroot/ http://deb.debian.org/debian >> >> # provide {dev,proc} for chroot >> sudo mount --rbind /dev ./bullseye-chroot/dev/ >> sudo mount --make-rslave ./bullseye-chroot/dev/ >> sudo mount -t proc proc ./bullseye-chroot/proc/ >> >> # enter chroot >> sudo chroot ./bullseye-chroot >> >> # Install build tools >> apt install build-essential bc flex bison git libelf-dev libssl-dev \ >> docutils-common rsync curl zstd qemu-system-x86 sudo cmake \ >> libdw-dev lsb-release wget software-properties-common gnupg e2fsprogs >> >> # Install fresh clang-18 snapshot, the llvm.sh sets up some repos >> curl https://apt.llvm.org/llvm.sh --output /tmp/llvm.sh >> bash /tmp/llvm.sh 18 >> apt install clang-tools-18 >> ln -s /usr/bin/clang-18 /usr/bin/clang >> ln -s /usr/bin/llvm-strip-18 /usr/bin/llvm-strip >> >> # that would be /root inside chroot >> cd $HOME >> >> # Get and compile pahole, use instructions from: >> # https://git.kernel.org/pub/scm/devel/pahole/pahole.git/about/ >> git clone https://git.kernel.org/pub/scm/devel/pahole/pahole.git >> cd pahole >> git submodule update --init --recursive >> mkdir build >> cd build >> cmake -D__LIB=lib .. >> make -j >> # make it available system-wide >> ln -s $(realpath pahole) /usr/local/bin/ >> >> cd $HOME >> git clone --depth=1 https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git >> >> # Run vmtests, this should download rootfs, build kernel and tests, run test_verifier >> # vmtest.sh would ask for root password to mount rootfs image >> cd bpf-next/tools/testing/selftests/bpf >> ./vmtest.sh -- ./test_verifier >> >> # And now run test_progs >> ./vmtest.sh -- ./test_progs >> >> # One can filter tests too >> ./vmtest.sh -- ./test_progs -t ringbuf >>
On Thu, 2024-09-12 at 22:40 +0800, lonial con wrote: > Hi, > > I tried to build this environment, but it seems that it needs kvm > support. For me, it is very troublesome to prepare a kvm environment. > So could you please write this selftest? Ok, no problem. > > Thanks, > Lonial Con [...]
On Thu, 2024-09-12 at 22:40 +0800, lonial con wrote: > Hi, > > I tried to build this environment, but it seems that it needs kvm > support. For me, it is very troublesome to prepare a kvm environment. > So could you please write this selftest? Please find the patch for test in the attachment. Please submit a v2 as a patch-set of two parts: - first patch: your fix - second patch: my test Also, please make sure to use up to date bpf-next kernel tree, your patch changes function find_equal_scalars(), this function was renamed to sync_linked_regs() some time ago. So the updated fix should look like: @@ -15349,8 +15349,12 @@ static void sync_linked_regs(struct bpf_verifier_state *vstate, struct bpf_reg_s continue; if ((!(reg->id & BPF_ADD_CONST) && !(known_reg->id & BPF_ADD_CONST)) || reg->off == known_reg->off) { + s32 saved_subreg_def = reg->subreg_def; + copy_register_state(reg, known_reg); + reg->subreg_def = saved_subreg_def; } else { + s32 saved_subreg_def = reg->subreg_def; s32 saved_off = reg->off; fake_reg.type = SCALAR_VALUE; @@ -15363,6 +15367,8 @@ static void sync_linked_regs(struct bpf_verifier_state *vstate, struct bpf_reg_s * otherwise another sync_linked_regs() will be incorrect. */ reg->off = saved_off; + /* TODO: describe why */ + reg->subreg_def = saved_subreg_def; scalar32_min_max_add(reg, &fake_reg); scalar_min_max_add(reg, &fake_reg); For illustrative purposes, you might refer to the test case in the commit message for the fix. (You can actually run it w/o KVM, it would be slower but otherwise should work). W/o your fix the test case is miscompiled as follows: call %[bpf_ktime_get_ns]; call unknown r0 &= 0x7fffffff; after verifier r0 &= 2147483647 w1 = w0; rewrites w1 = w0 if w0 < 10 goto +0; --------------> r11 = 794195110 r1 >>= 32; r11 <<= 32 r0 = r1; r1 |= r11 exit; if w0 < 0xa goto pc+0 r1 >>= 32 r0 = r1 exit Leaving return value undefined. [...]
On Thu, 2024-09-12 at 16:36 -0700, Eduard Zingerman wrote: > On Thu, 2024-09-12 at 22:40 +0800, lonial con wrote: > > Hi, > > > > I tried to build this environment, but it seems that it needs kvm > > support. For me, it is very troublesome to prepare a kvm environment. > > So could you please write this selftest? > > Please find the patch for test in the attachment. > Please submit a v2 as a patch-set of two parts: > - first patch: your fix > - second patch: my test Hi Lonial, Do you plan to proceed with this fix? [...]
Hi Eduard, Sorry, I was on vacation recently and didn't reply to emails in time. Could you please submit this patch directly? Because I am on vacation and don't have my computer with me. Thanks. Eduard Zingerman <eddyz87@gmail.com> 于2024年9月24日周二 16:12写道： > > On Thu, 2024-09-12 at 16:36 -0700, Eduard Zingerman wrote: > > On Thu, 2024-09-12 at 22:40 +0800, lonial con wrote: > > > Hi, > > > > > > I tried to build this environment, but it seems that it needs kvm > > > support. For me, it is very troublesome to prepare a kvm environment. > > > So could you please write this selftest? > > > > Please find the patch for test in the attachment. > > Please submit a v2 as a patch-set of two parts: > > - first patch: your fix > > - second patch: my test > > Hi Lonial, > > Do you plan to proceed with this fix? > > [...] >
On Tue, 2024-09-24 at 21:40 +0800, lonial con wrote: > Hi Eduard, > > Sorry, I was on vacation recently and didn't reply to emails in time. > Could you please submit this patch directly? Because I am on vacation > and don't have my computer with me. Sure, thanks again for the fix. Have a good vacation.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index d852009..1e01b7f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -15101,7 +15101,9 @@ static void find_equal_scalars(struct bpf_verifier_state *vstate, continue; if ((!(reg->id & BPF_ADD_CONST) && !(known_reg->id & BPF_ADD_CONST)) || reg->off == known_reg->off) { + s32 subreg_def = reg->subreg_def; copy_register_state(reg, known_reg); + reg->subreg_def = subreg_def; } else { s32 saved_off = reg->off; @@ -15109,7 +15111,9 @@ static void find_equal_scalars(struct bpf_verifier_state *vstate, __mark_reg_known(&fake_reg, (s32)reg->off - (s32)known_reg->off); /* reg = known_reg; reg += delta */ + s32 subreg_def = reg->subreg_def; copy_register_state(reg, known_reg); + reg->subreg_def = subreg_def; /* * Must preserve off, id and add_const flag, * otherwise another find_equal_scalars() will be incorrect.
In find_equal_scalars(), it should not copy the reg->subreg_def, otherwise a bug will occur when the program flag has BPF_F_TEST_RND_HI32. Reported-by: Lonial Con <kongln9170@gmail.com> Signed-off-by: Lonial Con <kongln9170@gmail.com> --- kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)