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

From: KP Singh <kpsingh@kernel.org>
To: Yonghong Song <yhs@fb.com>
Cc: bpf <bpf@vger.kernel.org>, 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: Tue, 12 Jan 2021 08:35:39 +0100	[thread overview]
Message-ID: <CACYkzJ4+H1XLF4DW7+0K+zFCFtzkdZQQAzk_UWsj5Bs9HaTa=Q@mail.gmail.com> (raw)
In-Reply-To: <317e2cb6-3774-b343-d93b-5b6f1d41b97e@fb.com>

On Tue, Jan 12, 2021 at 8:25 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> 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>
>
> > ---

[...]

> > @@ -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).

You are right, I will send in a v3 with this fixed and also add your Acks.
Thanks!

>
> >               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;
> > +
> [...]