From: Yonghong Song <yhs@fb.com>
To: KP Singh <kpsingh@kernel.org>, <bpf@vger.kernel.org>
Cc: Gilad Reti <gilad.reti@gmail.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <kafai@fb.com>
Subject: Re: [PATCH bpf v2 1/3] bpf: update local storage test to check handling of null ptrs
Date: Mon, 11 Jan 2021 23:24:41 -0800 [thread overview]
Message-ID: <317e2cb6-3774-b343-d93b-5b6f1d41b97e@fb.com> (raw)
In-Reply-To: <20210111212340.86393-2-kpsingh@kernel.org>
On 1/11/21 1:23 PM, KP Singh 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>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
Ack with one nit below.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> .../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..338475fe9ffb 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))
> + mkdtemp(tmp_dir_path);
> + if (CHECK(errno < 0, "mkdtemp", "unable to create tmpdir: %d\n", errno))
I think checking mkdtemp return value is more reliable than checking
errno. It is possible mkdtemp returns 0 and errno is not 0 (inheritted
from previous syscall).
> 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;
> +
[...]
next prev parent reply other threads:[~2021-01-12 7:25 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-11 21:23 [PATCH bpf v2 0/3] Fix local storage helper OOPs KP Singh
2021-01-11 21:23 ` [PATCH bpf v2 1/3] bpf: update local storage test to check handling of null ptrs KP Singh
2021-01-12 7:24 ` Yonghong Song [this message]
2021-01-12 7:35 ` KP Singh
2021-01-11 21:23 ` [PATCH bpf v2 2/3] bpf: local storage helpers should check nullness of owner ptr passed KP Singh
2021-01-12 7:25 ` Yonghong Song
2021-01-11 21:23 ` [PATCH bpf v2 3/3] bpf: Fix typo in bpf_inode_storage.c KP Singh
2021-01-12 7:26 ` Yonghong Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=317e2cb6-3774-b343-d93b-5b6f1d41b97e@fb.com \
--to=yhs@fb.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=gilad.reti@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.