Re: [PATCHv2 bpf] bpf: Add extra path pointer check to d_path helper

From: Yonghong Song <yhs@meta.com>
To: Jiri Olsa <jolsa@kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>
Cc: stable@vger.kernel.org, Stanislav Fomichev <sdf@google.com>,
	Anastasios Papagiannis <tasos.papagiannnis@gmail.com>,
	bpf@vger.kernel.org, Martin KaFai Lau <kafai@fb.com>,
	Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>, Hao Luo <haoluo@google.com>
Subject: Re: [PATCHv2 bpf] bpf: Add extra path pointer check to d_path helper
Date: Tue, 6 Jun 2023 14:07:58 -0700	[thread overview]
Message-ID: <cfcb1518-f13f-98c0-d051-0e3f904295bc@meta.com> (raw)
In-Reply-To: <20230606181714.532998-1-jolsa@kernel.org>


On 6/6/23 11:17 AM, Jiri Olsa wrote:
> Anastasios reported crash on stable 5.15 kernel with following
> bpf attached to lsm hook:
> 
>    SEC("lsm.s/bprm_creds_for_exec")
>    int BPF_PROG(bprm_creds_for_exec, struct linux_binprm *bprm)
>    {
>            struct path *path = &bprm->executable->f_path;
>            char p[128] = { 0 };
> 
>            bpf_d_path(path, p, 128);
>            return 0;
>    }
> 
> but bprm->executable can be NULL, so bpf_d_path call will crash:
> 
>    BUG: kernel NULL pointer dereference, address: 0000000000000018
>    #PF: supervisor read access in kernel mode
>    #PF: error_code(0x0000) - not-present page
>    PGD 0 P4D 0
>    Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC NOPTI
>    ...
>    RIP: 0010:d_path+0x22/0x280
>    ...
>    Call Trace:
>     <TASK>
>     bpf_d_path+0x21/0x60
>     bpf_prog_db9cf176e84498d9_bprm_creds_for_exec+0x94/0x99
>     bpf_trampoline_6442506293_0+0x55/0x1000
>     bpf_lsm_bprm_creds_for_exec+0x5/0x10
>     security_bprm_creds_for_exec+0x29/0x40
>     bprm_execve+0x1c1/0x900
>     do_execveat_common.isra.0+0x1af/0x260
>     __x64_sys_execve+0x32/0x40
> 
> It's problem for all stable trees with bpf_d_path helper, which was
> added in 5.9.
> 
> This issue is fixed in current bpf code, where we identify and mark
> trusted pointers, so the above code would fail even to load.
> 
> For the sake of the stable trees and to workaround potentially broken
> verifier in the future, adding the code that reads the path object from
> the passed pointer and verifies it's valid in kernel space.
> 
> Cc: stable@vger.kernel.org # v5.9+
> Fixes: 6e22ab9da793 ("bpf: Add d_path helper")
> Acked-by: Stanislav Fomichev <sdf@google.com>
> Suggested-by: Alexei Starovoitov <ast@kernel.org>
> Reported-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Yonghong Song <yhs@fb.com>