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
Date: Sun, 24 Mar 2024 10:44:08 +0000 [thread overview]
Message-ID: <mb61ple677vuv.fsf@gmail.com> (raw)
In-Reply-To: <CAADnVQJhxQV0xBJDWVkTGXJ+XYpsq5kFmMsuMEHBEaqUmkh0iw@mail.gmail.com>
Alexei Starovoitov <alexei.starovoitov@gmail•com> writes:
> On Fri, Mar 22, 2024 at 9:28 AM Daniel Borkmann <daniel@iogearbox•net> wrote:
>>
>> On 3/22/24 4:05 PM, Puranjay Mohan wrote:
>> [...]
>> >>> + /* Make it impossible to de-reference a userspace address */
>> >>> + if (BPF_CLASS(insn->code) == BPF_LDX &&
>> >>> + (BPF_MODE(insn->code) == BPF_PROBE_MEM ||
>> >>> + BPF_MODE(insn->code) == BPF_PROBE_MEMSX)) {
>> >>> + struct bpf_insn *patch = &insn_buf[0];
>> >>> + u64 uaddress_limit = bpf_arch_uaddress_limit();
>> >>> +
>> >>> + if (!uaddress_limit)
>> >>> + goto next_insn;
>> >>> +
>> >>> + *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
>> >>> + if (insn->off)
>> >>> + *patch++ = BPF_ALU64_IMM(BPF_ADD, BPF_REG_AX, insn->off);
>> >>> + *patch++ = BPF_ALU64_IMM(BPF_RSH, BPF_REG_AX, 32);
>> >>> + *patch++ = BPF_JMP_IMM(BPF_JLE, BPF_REG_AX, uaddress_limit >> 32, 2);
>> >>> + *patch++ = *insn;
>> >>> + *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
>> >>> + *patch++ = 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) 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_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 this
>> > page. I am not sure if that is acceptable.
>>
>> The bpf_probe_read_kernel() does handle it fine via copy_from_kernel_nofault().
>>
>> 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 means 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.
next prev parent reply other threads:[~2024-03-24 10:44 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-21 12:46 [PATCH bpf v4] bpf: verifier: prevent userspace memory access Puranjay Mohan
2024-03-22 2:24 ` Ilya Leoshkevich
2024-03-22 14:46 ` Daniel Borkmann
2024-03-22 15:05 ` Puranjay Mohan
2024-03-22 16:28 ` Daniel Borkmann
2024-03-22 16:53 ` Puranjay Mohan
2024-03-23 19:13 ` Alexei Starovoitov
2024-03-24 10:44 ` Puranjay Mohan [this message]
2024-03-24 19:12 ` Alexei Starovoitov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mb61ple677vuv.fsf@gmail.com \
--to=puranjay12@gmail$(echo .)com \
--cc=alexei.starovoitov@gmail$(echo .)com \
--cc=andrii@kernel$(echo .)org \
--cc=ast@kernel$(echo .)org \
--cc=bp@alien8$(echo .)de \
--cc=bpf@vger$(echo .)kernel.org \
--cc=daniel@iogearbox$(echo .)net \
--cc=dave.hansen@linux$(echo .)intel.com \
--cc=davem@davemloft$(echo .)net \
--cc=dsahern@kernel$(echo .)org \
--cc=eddyz87@gmail$(echo .)com \
--cc=haoluo@google$(echo .)com \
--cc=hpa@zytor$(echo .)com \
--cc=iii@linux$(echo .)ibm.com \
--cc=jean-philippe@linaro$(echo .)org \
--cc=john.fastabend@gmail$(echo .)com \
--cc=jolsa@kernel$(echo .)org \
--cc=kpsingh@kernel$(echo .)org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=martin.lau@linux$(echo .)dev \
--cc=mingo@redhat$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=sdf@google$(echo .)com \
--cc=song@kernel$(echo .)org \
--cc=tglx@linutronix$(echo .)de \
--cc=x86@kernel$(echo .)org \
--cc=yonghong.song@linux$(echo .)dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox