* [PATCH bpf v3 0/3] Fix local storage helper OOPs
@ 2021-01-12 7:55 KP Singh
2021-01-12 7:55 ` [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs KP Singh
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: KP Singh @ 2021-01-12 7:55 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau
# v2 -> v3
* Checking the return value of mkdtemp intead of errno
* Added Yonghong's Acks
It was noted in
https://lore.kernel.org/bpf/CACYkzJ55X8Tp2q4+EFf2hOM_Lysoim1xJY1YdA3k=T3woMW6mg@mail.gmail.com/T/#t
that the local storage helpers do not handle null owner pointers
correctly. This patch fixes the task and inode storage helpers with a
null check. In order to keep the check explicit, it's kept in the body
of the helpers similar to sk_storage and also fixes a minor typo in
bpf_inode_storage.c [I did not add a fixes and reported tag to the
patch that fixes the typo since it's a non-functional change].
The series also incorporates the example posted by Gilad into the
selftest. The selftest, without the fix reproduces the oops and the
subsequent patch fixes it.
KP Singh (3):
bpf: update local storage test to check handling of null ptrs
bpf: local storage helpers should check nullness of owner ptr passed
bpf: Fix typo in bpf_inode_storage.c
kernel/bpf/bpf_inode_storage.c | 9 +-
kernel/bpf/bpf_task_storage.c | 5 +-
.../bpf/prog_tests/test_local_storage.c | 96 +++++--------------
.../selftests/bpf/progs/local_storage.c | 62 ++++++------
4 files changed, 71 insertions(+), 101 deletions(-)
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs 2021-01-12 7:55 [PATCH bpf v3 0/3] Fix local storage helper OOPs KP Singh @ 2021-01-12 7:55 ` KP Singh 2021-01-28 1:46 ` Andrii Nakryiko 2021-01-12 7:55 ` [PATCH bpf v3 2/3] bpf: local storage helpers should check nullness of owner ptr passed KP Singh ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: KP Singh @ 2021-01-12 7:55 UTC (permalink / raw) To: bpf Cc: Gilad Reti, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau It was found in [1] that bpf_inode_storage_get helper did not check the nullness of the passed owner ptr which caused an oops when dereferenced. This change incorporates the example suggested in [1] into the local storage selftest. The test is updated to create a temporary directory instead of just using a tempfile. In order to replicate the issue this copied rm binary is renamed tiggering the inode_rename with a null pointer for the new_inode. The logic to verify the setting and deletion of the inode local storage of the old inode is also moved to this LSM hook. The change also removes the copy_rm function and simply shells out to copy files and recursively delete directories and consolidates the logic of setting the initial inode storage to the bprm_committed_creds hook and removes the file_open hook. [1]: https://lore.kernel.org/bpf/CANaYP3HWkH91SN=wTNO9FL_2ztHfqcXKX38SSE-JJ2voh+vssw@mail.gmail.com Suggested-by: Gilad Reti <gilad.reti@gmail.com> Acked-by: Yonghong Song <yhs@fb.com> Signed-off-by: KP Singh <kpsingh@kernel.org> --- .../bpf/prog_tests/test_local_storage.c | 96 +++++-------------- .../selftests/bpf/progs/local_storage.c | 62 ++++++------ 2 files changed, 61 insertions(+), 97 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/test_local_storage.c b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c index c0fe73a17ed1..3bfcf00c0a67 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_local_storage.c +++ b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c @@ -34,61 +34,6 @@ struct storage { struct bpf_spin_lock lock; }; -/* Copies an rm binary to a temp file. dest is a mkstemp template */ -static int copy_rm(char *dest) -{ - int fd_in, fd_out = -1, ret = 0; - struct stat stat; - char *buf = NULL; - - fd_in = open("/bin/rm", O_RDONLY); - if (fd_in < 0) - return -errno; - - fd_out = mkstemp(dest); - if (fd_out < 0) { - ret = -errno; - goto out; - } - - ret = fstat(fd_in, &stat); - if (ret == -1) { - ret = -errno; - goto out; - } - - buf = malloc(stat.st_blksize); - if (!buf) { - ret = -errno; - goto out; - } - - while (ret = read(fd_in, buf, stat.st_blksize), ret > 0) { - ret = write(fd_out, buf, ret); - if (ret < 0) { - ret = -errno; - goto out; - - } - } - if (ret < 0) { - ret = -errno; - goto out; - - } - - /* Set executable permission on the copied file */ - ret = chmod(dest, 0100); - if (ret == -1) - ret = -errno; - -out: - free(buf); - close(fd_in); - close(fd_out); - return ret; -} - /* Fork and exec the provided rm binary and return the exit code of the * forked process and its pid. */ @@ -168,9 +113,11 @@ static bool check_syscall_operations(int map_fd, int obj_fd) void test_test_local_storage(void) { - char tmp_exec_path[PATH_MAX] = "/tmp/copy_of_rmXXXXXX"; + char tmp_dir_path[64] = "/tmp/local_storageXXXXXX"; int err, serv_sk = -1, task_fd = -1, rm_fd = -1; struct local_storage *skel = NULL; + char tmp_exec_path[64]; + char cmd[256]; skel = local_storage__open_and_load(); if (CHECK(!skel, "skel_load", "lsm skeleton failed\n")) @@ -189,18 +136,24 @@ void test_test_local_storage(void) task_fd)) goto close_prog; - err = copy_rm(tmp_exec_path); - if (CHECK(err < 0, "copy_rm", "err %d errno %d\n", err, errno)) + if (CHECK(!mkdtemp(tmp_dir_path), "mkdtemp", + "unable to create tmpdir: %d\n", errno)) goto close_prog; + snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm", + tmp_dir_path); + snprintf(cmd, sizeof(cmd), "cp /bin/rm %s", tmp_exec_path); + if (CHECK_FAIL(system(cmd))) + goto close_prog_rmdir; + rm_fd = open(tmp_exec_path, O_RDONLY); if (CHECK(rm_fd < 0, "open", "failed to open %s err:%d, errno:%d", tmp_exec_path, rm_fd, errno)) - goto close_prog; + goto close_prog_rmdir; if (!check_syscall_operations(bpf_map__fd(skel->maps.inode_storage_map), rm_fd)) - goto close_prog; + goto close_prog_rmdir; /* Sets skel->bss->monitored_pid to the pid of the forked child * forks a child process that executes tmp_exec_path and tries to @@ -209,33 +162,36 @@ void test_test_local_storage(void) */ err = run_self_unlink(&skel->bss->monitored_pid, tmp_exec_path); if (CHECK(err != EPERM, "run_self_unlink", "err %d want EPERM\n", err)) - goto close_prog_unlink; + goto close_prog_rmdir; /* Set the process being monitored to be the current process */ skel->bss->monitored_pid = getpid(); - /* Remove the temporary created executable */ - err = unlink(tmp_exec_path); - if (CHECK(err != 0, "unlink", "unable to unlink %s: %d", tmp_exec_path, - errno)) - goto close_prog_unlink; + /* Move copy_of_rm to a new location so that it triggers the + * inode_rename LSM hook with a new_dentry that has a NULL inode ptr. + */ + snprintf(cmd, sizeof(cmd), "mv %s/copy_of_rm %s/check_null_ptr", + tmp_dir_path, tmp_dir_path); + if (CHECK_FAIL(system(cmd))) + goto close_prog_rmdir; CHECK(skel->data->inode_storage_result != 0, "inode_storage_result", "inode_local_storage not set\n"); serv_sk = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0); if (CHECK(serv_sk < 0, "start_server", "failed to start server\n")) - goto close_prog; + goto close_prog_rmdir; CHECK(skel->data->sk_storage_result != 0, "sk_storage_result", "sk_local_storage not set\n"); if (!check_syscall_operations(bpf_map__fd(skel->maps.sk_storage_map), serv_sk)) - goto close_prog; + goto close_prog_rmdir; -close_prog_unlink: - unlink(tmp_exec_path); +close_prog_rmdir: + snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp_dir_path); + system(cmd); close_prog: close(serv_sk); close(rm_fd); diff --git a/tools/testing/selftests/bpf/progs/local_storage.c b/tools/testing/selftests/bpf/progs/local_storage.c index 3e3de130f28f..95868bc7ada9 100644 --- a/tools/testing/selftests/bpf/progs/local_storage.c +++ b/tools/testing/selftests/bpf/progs/local_storage.c @@ -50,7 +50,6 @@ int BPF_PROG(unlink_hook, struct inode *dir, struct dentry *victim) __u32 pid = bpf_get_current_pid_tgid() >> 32; struct local_storage *storage; bool is_self_unlink; - int err; if (pid != monitored_pid) return 0; @@ -66,8 +65,27 @@ int BPF_PROG(unlink_hook, struct inode *dir, struct dentry *victim) return -EPERM; } - storage = bpf_inode_storage_get(&inode_storage_map, victim->d_inode, 0, - BPF_LOCAL_STORAGE_GET_F_CREATE); + return 0; +} + +SEC("lsm/inode_rename") +int BPF_PROG(inode_rename, struct inode *old_dir, struct dentry *old_dentry, + struct inode *new_dir, struct dentry *new_dentry, + unsigned int flags) +{ + __u32 pid = bpf_get_current_pid_tgid() >> 32; + struct local_storage *storage; + int err; + + /* new_dentry->d_inode can be NULL when the inode is renamed to a file + * that did not exist before. The helper should be able to handle this + * NULL pointer. + */ + bpf_inode_storage_get(&inode_storage_map, new_dentry->d_inode, 0, + BPF_LOCAL_STORAGE_GET_F_CREATE); + + storage = bpf_inode_storage_get(&inode_storage_map, old_dentry->d_inode, + 0, 0); if (!storage) return 0; @@ -76,7 +94,7 @@ int BPF_PROG(unlink_hook, struct inode *dir, struct dentry *victim) inode_storage_result = -1; bpf_spin_unlock(&storage->lock); - err = bpf_inode_storage_delete(&inode_storage_map, victim->d_inode); + err = bpf_inode_storage_delete(&inode_storage_map, old_dentry->d_inode); if (!err) inode_storage_result = err; @@ -133,37 +151,18 @@ int BPF_PROG(socket_post_create, struct socket *sock, int family, int type, return 0; } -SEC("lsm/file_open") -int BPF_PROG(file_open, struct file *file) -{ - __u32 pid = bpf_get_current_pid_tgid() >> 32; - struct local_storage *storage; - - if (pid != monitored_pid) - return 0; - - if (!file->f_inode) - return 0; - - storage = bpf_inode_storage_get(&inode_storage_map, file->f_inode, 0, - BPF_LOCAL_STORAGE_GET_F_CREATE); - if (!storage) - return 0; - - bpf_spin_lock(&storage->lock); - storage->value = DUMMY_STORAGE_VALUE; - bpf_spin_unlock(&storage->lock); - return 0; -} - /* This uses the local storage to remember the inode of the binary that a * process was originally executing. */ SEC("lsm/bprm_committed_creds") void BPF_PROG(exec, struct linux_binprm *bprm) { + __u32 pid = bpf_get_current_pid_tgid() >> 32; struct local_storage *storage; + if (pid != monitored_pid) + return; + storage = bpf_task_storage_get(&task_storage_map, bpf_get_current_task_btf(), 0, BPF_LOCAL_STORAGE_GET_F_CREATE); @@ -172,4 +171,13 @@ void BPF_PROG(exec, struct linux_binprm *bprm) storage->exec_inode = bprm->file->f_inode; bpf_spin_unlock(&storage->lock); } + + storage = bpf_inode_storage_get(&inode_storage_map, bprm->file->f_inode, + 0, BPF_LOCAL_STORAGE_GET_F_CREATE); + if (!storage) + return; + + bpf_spin_lock(&storage->lock); + storage->value = DUMMY_STORAGE_VALUE; + bpf_spin_unlock(&storage->lock); } -- 2.30.0.284.gd98b1dd5eaa7-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs 2021-01-12 7:55 ` [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs KP Singh @ 2021-01-28 1:46 ` Andrii Nakryiko 2021-02-01 1:09 ` KP Singh 0 siblings, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2021-01-28 1:46 UTC (permalink / raw) To: KP Singh Cc: bpf, Gilad Reti, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau On Mon, Jan 11, 2021 at 11:55 PM KP Singh <kpsingh@kernel.org> wrote: > > It was found in [1] that bpf_inode_storage_get helper did not check > the nullness of the passed owner ptr which caused an oops when > dereferenced. This change incorporates the example suggested in [1] into > the local storage selftest. > > The test is updated to create a temporary directory instead of just > using a tempfile. In order to replicate the issue this copied rm binary > is renamed tiggering the inode_rename with a null pointer for the > new_inode. The logic to verify the setting and deletion of the inode > local storage of the old inode is also moved to this LSM hook. > > The change also removes the copy_rm function and simply shells out > to copy files and recursively delete directories and consolidates the > logic of setting the initial inode storage to the bprm_committed_creds > hook and removes the file_open hook. > > [1]: https://lore.kernel.org/bpf/CANaYP3HWkH91SN=wTNO9FL_2ztHfqcXKX38SSE-JJ2voh+vssw@mail.gmail.com > > Suggested-by: Gilad Reti <gilad.reti@gmail.com> > Acked-by: Yonghong Song <yhs@fb.com> > Signed-off-by: KP Singh <kpsingh@kernel.org> > --- Hi KP, I'm getting a compilation warning when building selftests. Can you please take a look and send a fix? Thanks! /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c: In function ‘test_test_local_storage’: /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c:143:52: warning: ‘/copy_of_rm’ directive output may be truncated writing 11 bytes into a region of size between 1 and 64 [-Wformat-truncation=] 143 | snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm", | ^~~~~~~~~~~ /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c:143:2: note: ‘snprintf’ output between 12 and 75 bytes into a destination of size 64 143 | snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 144 | tmp_dir_path); | ~~~~~~~~~~~~~ > .../bpf/prog_tests/test_local_storage.c | 96 +++++-------------- > .../selftests/bpf/progs/local_storage.c | 62 ++++++------ > 2 files changed, 61 insertions(+), 97 deletions(-) > [...] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs 2021-01-28 1:46 ` Andrii Nakryiko @ 2021-02-01 1:09 ` KP Singh 2021-02-02 6:37 ` Andrii Nakryiko 0 siblings, 1 reply; 9+ messages in thread From: KP Singh @ 2021-02-01 1:09 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Gilad Reti, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau On Thu, Jan 28, 2021 at 2:46 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Jan 11, 2021 at 11:55 PM KP Singh <kpsingh@kernel.org> wrote: > > > > It was found in [1] that bpf_inode_storage_get helper did not check > > the nullness of the passed owner ptr which caused an oops when > > dereferenced. This change incorporates the example suggested in [1] into > > the local storage selftest. > > > > The test is updated to create a temporary directory instead of just > > using a tempfile. In order to replicate the issue this copied rm binary > > is renamed tiggering the inode_rename with a null pointer for the > > new_inode. The logic to verify the setting and deletion of the inode > > local storage of the old inode is also moved to this LSM hook. > > > > The change also removes the copy_rm function and simply shells out > > to copy files and recursively delete directories and consolidates the > > logic of setting the initial inode storage to the bprm_committed_creds > > hook and removes the file_open hook. > > > > [1]: https://lore.kernel.org/bpf/CANaYP3HWkH91SN=wTNO9FL_2ztHfqcXKX38SSE-JJ2voh+vssw@mail.gmail.com > > > > Suggested-by: Gilad Reti <gilad.reti@gmail.com> > > Acked-by: Yonghong Song <yhs@fb.com> > > Signed-off-by: KP Singh <kpsingh@kernel.org> > > --- > > Hi KP, > > I'm getting a compilation warning when building selftests. Can you > please take a look and send a fix? Thanks! > > /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c: > In function ‘test_test_local_storage’: > /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c:143:52: > warning: ‘/copy_of_rm’ directive output may be truncated writing 11 > bytes into a region of size between 1 and 64 [-Wformat-truncation=] > 143 | snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm", > | ^~~~~~~~~~~ > /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c:143:2: > note: ‘snprintf’ output between 12 and 75 bytes into a destination of > size 64 > 143 | snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm", > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 144 | tmp_dir_path); > | ~~~~~~~~~~~~~ > I don't seem to get this warning, so maybe we are using different compilers. Mine is gcc 10.2.1 20201224 (from debian) That said, I understand why it's complaining, it's for something that cannot really happen: tmp_dir_path cannot be 64 because we actually know its length so the tmp_exec_path cannot really overflow 64 bytes. Can you check if the following patch makes it go away? diff --git a/tools/testing/selftests/bpf/prog_tests/test_local_storage.c b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c index 3bfcf00c0a67..d2c16eaae367 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_local_storage.c +++ b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c @@ -113,7 +113,7 @@ static bool check_syscall_operations(int map_fd, int obj_fd) void test_test_local_storage(void) { - char tmp_dir_path[64] = "/tmp/local_storageXXXXXX"; + char tmp_dir_path[] = "/tmp/local_storageXXXXXX"; int err, serv_sk = -1, task_fd = -1, rm_fd = -1; struct local_storage *skel = NULL; char tmp_exec_path[64]; If so, I can send you a fix. - KP > > > .../bpf/prog_tests/test_local_storage.c | 96 +++++-------------- > > .../selftests/bpf/progs/local_storage.c | 62 ++++++------ > > 2 files changed, 61 insertions(+), 97 deletions(-) > > > > [...] ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs 2021-02-01 1:09 ` KP Singh @ 2021-02-02 6:37 ` Andrii Nakryiko 2021-02-02 7:10 ` Yonghong Song 0 siblings, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2021-02-02 6:37 UTC (permalink / raw) To: KP Singh Cc: bpf, Gilad Reti, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau On Sun, Jan 31, 2021 at 5:09 PM KP Singh <kpsingh@kernel.org> wrote: > > On Thu, Jan 28, 2021 at 2:46 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Mon, Jan 11, 2021 at 11:55 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > > It was found in [1] that bpf_inode_storage_get helper did not check > > > the nullness of the passed owner ptr which caused an oops when > > > dereferenced. This change incorporates the example suggested in [1] into > > > the local storage selftest. > > > > > > The test is updated to create a temporary directory instead of just > > > using a tempfile. In order to replicate the issue this copied rm binary > > > is renamed tiggering the inode_rename with a null pointer for the > > > new_inode. The logic to verify the setting and deletion of the inode > > > local storage of the old inode is also moved to this LSM hook. > > > > > > The change also removes the copy_rm function and simply shells out > > > to copy files and recursively delete directories and consolidates the > > > logic of setting the initial inode storage to the bprm_committed_creds > > > hook and removes the file_open hook. > > > > > > [1]: https://lore.kernel.org/bpf/CANaYP3HWkH91SN=wTNO9FL_2ztHfqcXKX38SSE-JJ2voh+vssw@mail.gmail.com > > > > > > Suggested-by: Gilad Reti <gilad.reti@gmail.com> > > > Acked-by: Yonghong Song <yhs@fb.com> > > > Signed-off-by: KP Singh <kpsingh@kernel.org> > > > --- > > > > Hi KP, > > > > I'm getting a compilation warning when building selftests. Can you > > please take a look and send a fix? Thanks! > > > > /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c: > > In function ‘test_test_local_storage’: > > /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c:143:52: > > warning: ‘/copy_of_rm’ directive output may be truncated writing 11 > > bytes into a region of size between 1 and 64 [-Wformat-truncation=] > > 143 | snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm", > > | ^~~~~~~~~~~ > > /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c:143:2: > > note: ‘snprintf’ output between 12 and 75 bytes into a destination of > > size 64 > > 143 | snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm", > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 144 | tmp_dir_path); > > | ~~~~~~~~~~~~~ > > > > I don't seem to get this warning, so maybe we are using different compilers. > > Mine is gcc 10.2.1 20201224 (from debian) Funny enough, but I can't repro it locally anymore. I have gcc 10.2.0. But your suggested fix below does look like a correct one, so feel free to send it over, thanks! > > That said, I understand why it's complaining, it's for something that > cannot really happen: > > tmp_dir_path cannot be 64 because we actually know its length so the > tmp_exec_path cannot really overflow 64 bytes. > > Can you check if the following patch makes it go away? > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_local_storage.c > b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c > index 3bfcf00c0a67..d2c16eaae367 100644 > --- a/tools/testing/selftests/bpf/prog_tests/test_local_storage.c > +++ b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c > @@ -113,7 +113,7 @@ static bool check_syscall_operations(int map_fd, int obj_fd) > > void test_test_local_storage(void) > { > - char tmp_dir_path[64] = "/tmp/local_storageXXXXXX"; > + char tmp_dir_path[] = "/tmp/local_storageXXXXXX"; > int err, serv_sk = -1, task_fd = -1, rm_fd = -1; > struct local_storage *skel = NULL; > char tmp_exec_path[64]; > > If so, I can send you a fix. > > - KP > > > > > > .../bpf/prog_tests/test_local_storage.c | 96 +++++-------------- > > > .../selftests/bpf/progs/local_storage.c | 62 ++++++------ > > > 2 files changed, 61 insertions(+), 97 deletions(-) > > > > > > > [...] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs 2021-02-02 6:37 ` Andrii Nakryiko @ 2021-02-02 7:10 ` Yonghong Song 0 siblings, 0 replies; 9+ messages in thread From: Yonghong Song @ 2021-02-02 7:10 UTC (permalink / raw) To: Andrii Nakryiko, KP Singh Cc: bpf, Gilad Reti, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau On 2/1/21 10:37 PM, Andrii Nakryiko wrote: > On Sun, Jan 31, 2021 at 5:09 PM KP Singh <kpsingh@kernel.org> wrote: >> >> On Thu, Jan 28, 2021 at 2:46 AM Andrii Nakryiko >> <andrii.nakryiko@gmail.com> wrote: >>> >>> On Mon, Jan 11, 2021 at 11:55 PM KP Singh <kpsingh@kernel.org> wrote: >>>> >>>> It was found in [1] that bpf_inode_storage_get helper did not check >>>> the nullness of the passed owner ptr which caused an oops when >>>> dereferenced. This change incorporates the example suggested in [1] into >>>> the local storage selftest. >>>> >>>> The test is updated to create a temporary directory instead of just >>>> using a tempfile. In order to replicate the issue this copied rm binary >>>> is renamed tiggering the inode_rename with a null pointer for the >>>> new_inode. The logic to verify the setting and deletion of the inode >>>> local storage of the old inode is also moved to this LSM hook. >>>> >>>> The change also removes the copy_rm function and simply shells out >>>> to copy files and recursively delete directories and consolidates the >>>> logic of setting the initial inode storage to the bprm_committed_creds >>>> hook and removes the file_open hook. >>>> >>>> [1]: https://lore.kernel.org/bpf/CANaYP3HWkH91SN=wTNO9FL_2ztHfqcXKX38SSE-JJ2voh+vssw@mail.gmail.com >>>> >>>> Suggested-by: Gilad Reti <gilad.reti@gmail.com> >>>> Acked-by: Yonghong Song <yhs@fb.com> >>>> Signed-off-by: KP Singh <kpsingh@kernel.org> >>>> --- >>> >>> Hi KP, >>> >>> I'm getting a compilation warning when building selftests. Can you >>> please take a look and send a fix? Thanks! >>> >>> /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c: >>> In function ‘test_test_local_storage’: >>> /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c:143:52: >>> warning: ‘/copy_of_rm’ directive output may be truncated writing 11 >>> bytes into a region of size between 1 and 64 [-Wformat-truncation=] >>> 143 | snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm", >>> | ^~~~~~~~~~~ >>> /data/users/andriin/linux/tools/testing/selftests/bpf/prog_tests/test_local_storage.c:143:2: >>> note: ‘snprintf’ output between 12 and 75 bytes into a destination of >>> size 64 >>> 143 | snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm", >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> 144 | tmp_dir_path); >>> | ~~~~~~~~~~~~~ >>> >> >> I don't seem to get this warning, so maybe we are using different compilers. >> >> Mine is gcc 10.2.1 20201224 (from debian) > > Funny enough, but I can't repro it locally anymore. I have gcc 10.2.0. > But your suggested fix below does look like a correct one, so feel > free to send it over, thanks! > >> >> That said, I understand why it's complaining, it's for something that >> cannot really happen: >> >> tmp_dir_path cannot be 64 because we actually know its length so the >> tmp_exec_path cannot really overflow 64 bytes. >> >> Can you check if the following patch makes it go away? >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/test_local_storage.c >> b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c >> index 3bfcf00c0a67..d2c16eaae367 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/test_local_storage.c >> +++ b/tools/testing/selftests/bpf/prog_tests/test_local_storage.c >> @@ -113,7 +113,7 @@ static bool check_syscall_operations(int map_fd, int obj_fd) >> >> void test_test_local_storage(void) >> { >> - char tmp_dir_path[64] = "/tmp/local_storageXXXXXX"; >> + char tmp_dir_path[] = "/tmp/local_storageXXXXXX"; >> int err, serv_sk = -1, task_fd = -1, rm_fd = -1; >> struct local_storage *skel = NULL; >> char tmp_exec_path[64]; >> >> If so, I can send you a fix. I have gcc 8.2.1 which can reproduce the issue. With the above fix, the warning is gone. So yes, please send a fix. Thanks! >> >> - KP >> >>> >>>> .../bpf/prog_tests/test_local_storage.c | 96 +++++-------------- >>>> .../selftests/bpf/progs/local_storage.c | 62 ++++++------ >>>> 2 files changed, 61 insertions(+), 97 deletions(-) >>>> >>> >>> [...] ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf v3 2/3] bpf: local storage helpers should check nullness of owner ptr passed 2021-01-12 7:55 [PATCH bpf v3 0/3] Fix local storage helper OOPs KP Singh 2021-01-12 7:55 ` [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs KP Singh @ 2021-01-12 7:55 ` KP Singh 2021-01-12 7:55 ` [PATCH bpf v3 3/3] bpf: Fix typo in bpf_inode_storage.c KP Singh 2021-01-12 15:20 ` [PATCH bpf v3 0/3] Fix local storage helper OOPs patchwork-bot+netdevbpf 3 siblings, 0 replies; 9+ messages in thread From: KP Singh @ 2021-01-12 7:55 UTC (permalink / raw) To: bpf Cc: Gilad Reti, Martin KaFai Lau, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko The verifier allows ARG_PTR_TO_BTF_ID helper arguments to be NULL, so helper implementations need to check this before dereferencing them. This was already fixed for the socket storage helpers but not for task and inode. The issue can be reproduced by attaching an LSM program to inode_rename hook (called when moving files) which tries to get the inode of the new file without checking for its nullness and then trying to move an existing file to a new path: mv existing_file new_file_does_not_exist The report including the sample program and the steps for reproducing the bug: https://lore.kernel.org/bpf/CANaYP3HWkH91SN=wTNO9FL_2ztHfqcXKX38SSE-JJ2voh+vssw@mail.gmail.com Fixes: 4cf1bc1f1045 ("bpf: Implement task local storage") Fixes: 8ea636848aca ("bpf: Implement bpf_local_storage for inodes") Reported-by: Gilad Reti <gilad.reti@gmail.com> Acked-by: Martin KaFai Lau <kafai@fb.com> Acked-by: Yonghong Song <yhs@fb.com> Signed-off-by: KP Singh <kpsingh@kernel.org> --- kernel/bpf/bpf_inode_storage.c | 5 ++++- kernel/bpf/bpf_task_storage.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c index 6edff97ad594..dbc1dbdd2cbf 100644 --- a/kernel/bpf/bpf_inode_storage.c +++ b/kernel/bpf/bpf_inode_storage.c @@ -176,7 +176,7 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode, * bpf_local_storage_update expects the owner to have a * valid storage pointer. */ - if (!inode_storage_ptr(inode)) + if (!inode || !inode_storage_ptr(inode)) return (unsigned long)NULL; sdata = inode_storage_lookup(inode, map, true); @@ -200,6 +200,9 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode, BPF_CALL_2(bpf_inode_storage_delete, struct bpf_map *, map, struct inode *, inode) { + if (!inode) + return -EINVAL; + /* This helper must only called from where the inode is gurranteed * to have a refcount and cannot be freed. */ diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c index 4ef1959a78f2..e0da0258b732 100644 --- a/kernel/bpf/bpf_task_storage.c +++ b/kernel/bpf/bpf_task_storage.c @@ -218,7 +218,7 @@ BPF_CALL_4(bpf_task_storage_get, struct bpf_map *, map, struct task_struct *, * bpf_local_storage_update expects the owner to have a * valid storage pointer. */ - if (!task_storage_ptr(task)) + if (!task || !task_storage_ptr(task)) return (unsigned long)NULL; sdata = task_storage_lookup(task, map, true); @@ -243,6 +243,9 @@ BPF_CALL_4(bpf_task_storage_get, struct bpf_map *, map, struct task_struct *, BPF_CALL_2(bpf_task_storage_delete, struct bpf_map *, map, struct task_struct *, task) { + if (!task) + return -EINVAL; + /* This helper must only be called from places where the lifetime of the task * is guaranteed. Either by being refcounted or by being protected * by an RCU read-side critical section. -- 2.30.0.284.gd98b1dd5eaa7-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf v3 3/3] bpf: Fix typo in bpf_inode_storage.c 2021-01-12 7:55 [PATCH bpf v3 0/3] Fix local storage helper OOPs KP Singh 2021-01-12 7:55 ` [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs KP Singh 2021-01-12 7:55 ` [PATCH bpf v3 2/3] bpf: local storage helpers should check nullness of owner ptr passed KP Singh @ 2021-01-12 7:55 ` KP Singh 2021-01-12 15:20 ` [PATCH bpf v3 0/3] Fix local storage helper OOPs patchwork-bot+netdevbpf 3 siblings, 0 replies; 9+ messages in thread From: KP Singh @ 2021-01-12 7:55 UTC (permalink / raw) To: bpf Cc: Andrii Nakryiko, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau Fix "gurranteed" -> "guaranteed" in bpf_inode_storage.c Suggested-by: Andrii Nakryiko <andrii@kernel.org> Acked-by: Yonghong Song <yhs@fb.com> Signed-off-by: KP Singh <kpsingh@kernel.org> --- kernel/bpf/bpf_inode_storage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c index dbc1dbdd2cbf..2f0597320b6d 100644 --- a/kernel/bpf/bpf_inode_storage.c +++ b/kernel/bpf/bpf_inode_storage.c @@ -183,7 +183,7 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode, if (sdata) return (unsigned long)sdata->data; - /* This helper must only called from where the inode is gurranteed + /* This helper must only called from where the inode is guaranteed * to have a refcount and cannot be freed. */ if (flags & BPF_LOCAL_STORAGE_GET_F_CREATE) { @@ -203,7 +203,7 @@ BPF_CALL_2(bpf_inode_storage_delete, if (!inode) return -EINVAL; - /* This helper must only called from where the inode is gurranteed + /* This helper must only called from where the inode is guaranteed * to have a refcount and cannot be freed. */ return inode_storage_delete(inode, map); -- 2.30.0.284.gd98b1dd5eaa7-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf v3 0/3] Fix local storage helper OOPs 2021-01-12 7:55 [PATCH bpf v3 0/3] Fix local storage helper OOPs KP Singh ` (2 preceding siblings ...) 2021-01-12 7:55 ` [PATCH bpf v3 3/3] bpf: Fix typo in bpf_inode_storage.c KP Singh @ 2021-01-12 15:20 ` patchwork-bot+netdevbpf 3 siblings, 0 replies; 9+ messages in thread From: patchwork-bot+netdevbpf @ 2021-01-12 15:20 UTC (permalink / raw) To: KP Singh; +Cc: bpf, ast, daniel, andrii, kafai Hello: This series was applied to bpf/bpf.git (refs/heads/master): On Tue, 12 Jan 2021 07:55:22 +0000 you wrote: > # v2 -> v3 > > * Checking the return value of mkdtemp intead of errno > * Added Yonghong's Acks > > It was noted in > https://lore.kernel.org/bpf/CACYkzJ55X8Tp2q4+EFf2hOM_Lysoim1xJY1YdA3k=T3woMW6mg@mail.gmail.com/T/#t > that the local storage helpers do not handle null owner pointers > correctly. This patch fixes the task and inode storage helpers with a > null check. In order to keep the check explicit, it's kept in the body > of the helpers similar to sk_storage and also fixes a minor typo in > bpf_inode_storage.c [I did not add a fixes and reported tag to the > patch that fixes the typo since it's a non-functional change]. > > [...] Here is the summary with links: - [bpf,v3,1/3] bpf: update local storage test to check handling of null ptrs https://git.kernel.org/bpf/bpf/c/2f94ac191846 - [bpf,v3,2/3] bpf: local storage helpers should check nullness of owner ptr passed https://git.kernel.org/bpf/bpf/c/1a9c72ad4c26 - [bpf,v3,3/3] bpf: Fix typo in bpf_inode_storage.c https://git.kernel.org/bpf/bpf/c/84d571d46c70 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-02 7:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-12 7:55 [PATCH bpf v3 0/3] Fix local storage helper OOPs KP Singh 2021-01-12 7:55 ` [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs KP Singh 2021-01-28 1:46 ` Andrii Nakryiko 2021-02-01 1:09 ` KP Singh 2021-02-02 6:37 ` Andrii Nakryiko 2021-02-02 7:10 ` Yonghong Song 2021-01-12 7:55 ` [PATCH bpf v3 2/3] bpf: local storage helpers should check nullness of owner ptr passed KP Singh 2021-01-12 7:55 ` [PATCH bpf v3 3/3] bpf: Fix typo in bpf_inode_storage.c KP Singh 2021-01-12 15:20 ` [PATCH bpf v3 0/3] Fix local storage helper OOPs patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).