[PATCH bpf v3 0/3] Fix local storage helper OOPs

[PATCH bpf v3 0/3] Fix local storage helper OOPs

[KP Singh]

# 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

[PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs

[KP Singh]

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

[PATCH bpf v3 2/3] bpf: local storage helpers should check nullness of owner ptr passed

[KP Singh]

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

[PATCH bpf v3 3/3] bpf: Fix typo in bpf_inode_storage.c

[KP Singh]

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

Re: [PATCH bpf v3 0/3] Fix local storage helper OOPs

[patchwork-bot+netdevbpf]

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

Re: [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs

[Andrii Nakryiko]

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(-)
>

[...]

Re: [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs

[KP Singh]

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(-)
> >
>
> [...]

Re: [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs

[Andrii Nakryiko]

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(-)
> > >
> >
> > [...]

Re: [PATCH bpf v3 1/3] bpf: update local storage test to check handling of null ptrs

[Yonghong Song]

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(-)
>>>>
>>>
>>> [...]