Received-SPF: pass (google.com: domain of linux-kernel+bounces-112819-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240321124640.8870-1-puranjay12@gmail.com> <9f2b63b5-569c-1e00-a635-93d9cd695517@iogearbox.net>
 <mb61p4jcyxq5m.fsf@gmail.com> <15ba79e3-14b2-d92e-3f94-e4f5f963e15d@iogearbox.net>
 <CAADnVQJhxQV0xBJDWVkTGXJ+XYpsq5kFmMsuMEHBEaqUmkh0iw@mail.gmail.com> <mb61ple677vuv.fsf@gmail.com>
In-Reply-To: <mb61ple677vuv.fsf@gmail.com>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Sun, 24 Mar 2024 12:12:00 -0700
Message-ID: <CAADnVQL3m_pDaTF9aL85Sz1Fr8UWcWmnKe2foqzVj35GiEofYA@mail.gmail.com>
Subject: Re: [PATCH bpf v4] bpf: verifier: prevent userspace memory access
To: Puranjay Mohan <puranjay12@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>, "David S. Miller" <davem@davemloft.net>, 
	David Ahern <dsahern@kernel.org>, Alexei Starovoitov <ast@kernel.org>, Andrii Nakryiko <andrii@kernel.org>, 
	Martin KaFai Lau <martin.lau@linux.dev>, Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>, 
	Yonghong Song <yonghong.song@linux.dev>, John Fastabend <john.fastabend@gmail.com>, 
	KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, 
	Jiri Olsa <jolsa@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, 
	Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, X86 ML <x86@kernel.org>, 
	"H. Peter Anvin" <hpa@zytor.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>, 
	Network Development <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>, 
	LKML <linux-kernel@vger.kernel.org>, Ilya Leoshkevich <iii@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Mar 24, 2024 at 3:44=E2=80=AFAM Puranjay Mohan <puranjay12@gmail.co=
m> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>
> > On Fri, Mar 22, 2024 at 9:28=E2=80=AFAM Daniel Borkmann <daniel@iogearb=
ox.net> wrote:
> >>
> >> On 3/22/24 4:05 PM, Puranjay Mohan wrote:
> >> [...]
> >> >>> +           /* Make it impossible to de-reference a userspace addr=
ess */
> >> >>> +           if (BPF_CLASS(insn->code) =3D=3D BPF_LDX &&
> >> >>> +               (BPF_MODE(insn->code) =3D=3D BPF_PROBE_MEM ||
> >> >>> +                BPF_MODE(insn->code) =3D=3D BPF_PROBE_MEMSX)) {
> >> >>> +                   struct bpf_insn *patch =3D &insn_buf[0];
> >> >>> +                   u64 uaddress_limit =3D bpf_arch_uaddress_limit=
();
> >> >>> +
> >> >>> +                   if (!uaddress_limit)
> >> >>> +                           goto next_insn;
> >> >>> +
> >> >>> +                   *patch++ =3D BPF_MOV64_REG(BPF_REG_AX, insn->s=
rc_reg);
> >> >>> +                   if (insn->off)
> >> >>> +                           *patch++ =3D BPF_ALU64_IMM(BPF_ADD, BP=
F_REG_AX, insn->off);
> >> >>> +                   *patch++ =3D BPF_ALU64_IMM(BPF_RSH, BPF_REG_AX=
, 32);
> >> >>> +                   *patch++ =3D BPF_JMP_IMM(BPF_JLE, BPF_REG_AX, =
uaddress_limit >> 32, 2);
> >> >>> +                   *patch++ =3D *insn;
> >> >>> +                   *patch++ =3D BPF_JMP_IMM(BPF_JA, 0, 0, 1);
> >> >>> +                   *patch++ =3D BPF_MOV64_IMM(insn->dst_reg, 0);
> >> >>
> >> >> But how does this address other cases where we could fault e.g. non=
-canonical,
> >> >> vsyscall page, etc? Technically, we would have to call to copy_from=
_kernel_nofault_allowed()
> >> >> to really address all the cases aside from the overflow (good catch=
 btw!) where kernel
> >> >> turns into user address.
> >> >
> >> > So, we are trying to ~simulate a call to
> >> > copy_from_kernel_nofault_allowed() here. If the address under
> >> > consideration is below TASK_SIZE (TASK_SIZE + 4GB to be precise) the=
n we
> >> > skip that load because that address could be mapped by the user.
> >> >
> >> > If the address is above TASK_SIZE + 4GB, we allow the load and it co=
uld
> >> > cause a fault if the address is invalid, non-canonical etc. Taking t=
he
> >> > fault is fine because JIT will add an exception table entry for
> >> > for that load with BPF_PBOBE_MEM.
> >>
> >> Are you sure? I don't think the kernel handles non-canonical fixup.
> >
> > I believe it handles it fine otherwise our selftest bpf_testmod_return_=
ptr:
> >    case 4: return (void *)(1ull << 60);    /* non-canonical and invalid=
 */
> > would have been crashing for the last 3 years,
> > since we've been running it.
> >
> >> > The vsyscall page is special, this approach skips all loads from thi=
s
> >> > page. I am not sure if that is acceptable.
> >>
> >> The bpf_probe_read_kernel() does handle it fine via copy_from_kernel_n=
ofault().
> >>
> >> So there is tail risk that BPF_PROBE_* could trigger a crash.
> >
> > For this patch let's do
> > return max(TASK_SIZE_MAX + PAGE_SIZE, VSYSCALL_ADDR)
> > to cover both with one check?
>
> I agree, will add this in the next version.

Sorry. I didn't look at actual value when I suggested this.
Let's think how to check for them with one conditional branch.
Maybe we can REG_AX >>=3D 24 instead of 32
and do some clever math, since we need to:
addr < 0x7ffffful || addr =3D=3D 0xfffffffffful
?
If we have to do two branches that's fine for now,
but we probably need to leave it to JITs,
since they can emit two faster conditional moves and use
single conditional jump or
introduce new pseudo bpf insns equivalent to x86 set[ae|ne|..]