Received-SPF: pass (google.com: domain of linux-kernel+bounces-112635-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Puranjay Mohan <puranjay12@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>, Daniel Borkmann
 <daniel@iogearbox.net>
Cc: "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>
Subject: Re: [PATCH bpf v4] bpf: verifier: prevent userspace memory access
In-Reply-To: <CAADnVQJhxQV0xBJDWVkTGXJ+XYpsq5kFmMsuMEHBEaqUmkh0iw@mail.gmail.com>
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>
Date: Sun, 24 Mar 2024 10:44:08 +0000
Message-ID: <mb61ple677vuv.fsf@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Fri, Mar 22, 2024 at 9:28=E2=80=AFAM Daniel Borkmann <daniel@iogearbox=
net> wrote:
>>
>> On 3/22/24 4:05 PM, Puranjay Mohan wrote:
>> [...]
>> >>> +           /* Make it impossible to de-reference a userspace addres=
s */
>> >>> +           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->src=
_reg);
>> >>> +                   if (insn->off)
>> >>> +                           *patch++ =3D BPF_ALU64_IMM(BPF_ADD, BPF_=
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, ua=
ddress_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-c=
anonical,
>> >> vsyscall page, etc? Technically, we would have to call to copy_from_k=
ernel_nofault_allowed()
>> >> to really address all the cases aside from the overflow (good catch b=
tw!) 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) then =
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 could
>> > cause a fault if the address is invalid, non-canonical etc. Taking the
>> > 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_pt=
r:
>    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 this
>> > page. I am not sure if that is acceptable.
>>
>> The bpf_probe_read_kernel() does handle it fine via copy_from_kernel_nof=
ault().
>>
>> 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.

>> Other archs might
>> have other quirks, e.g. in case of loongarch it says highest bit set mea=
ns kernel
>> space.
>
> let's tackle loongarch with whatever quirks it has separately.

Yes, having the current patch will not break loongarch, it will help it
skip some userspace addresses. We can later implement
bpf_arch_uaddress_limit() in loongarch JIT to handle its specific
quirks.