Skip to content

Commit

Permalink
Merge branch 'fix-the-read-of-vsyscall-page-through-bpf'
Browse files Browse the repository at this point in the history
Hou Tao says:

====================
Fix the read of vsyscall page through bpf

From: Hou Tao <[email protected]>

Hi,

As reported by syzboot [1] and [2], when trying to read vsyscall page
by using bpf_probe_read_kernel() or bpf_probe_read(), oops may happen.

Thomas Gleixner had proposed a test patch [3], but it seems that no
formal patch is posted after about one month [4], so I post it instead
and add an Originally-by tag in patch #2.

Patch #1 makes is_vsyscall_vaddr() being a common helper. Patch #2 fixes
the problem by disallowing vsyscall page read for
copy_from_kernel_nofault(). Patch #3 adds one test case to ensure the
read of vsyscall page through bpf is rejected. Please see individual
patches for more details.

Comments are always welcome.

[1]: https://lore.kernel.org/bpf/CAG48ez06TZft=ATH1qh2c5mpS5BT8UakwNkzi6nvK5_djC-4Nw@mail.gmail.com/
[2]: https://lore.kernel.org/bpf/CABOYnLynjBoFZOf3Z4BhaZkc5hx_kHfsjiW+UWLoB=w33LvScw@mail.gmail.com/
[3]: https://lore.kernel.org/bpf/87r0jwquhv.ffs@tglx/
[4]: https://lore.kernel.org/bpf/[email protected]/

Change Log:
v3:
 * rephrase commit message for patch #1 & #2 (Sohil)
 * reword comments in copy_from_kernel_nofault_allowed() (Sohil)
 * add Rvb tag for patch #1 and Acked-by tag for patch #3 (Sohil, Yonghong)

v2: https://lore.kernel.org/bpf/[email protected]/
  * move is_vsyscall_vaddr to asm/vsyscall.h instead (Sohil)
  * elaborate on the reason for disallowing of vsyscall page read in
    copy_from_kernel_nofault_allowed() (Sohil)
  * update the commit message of patch #2 to more clearly explain how
    the oops occurs. (Sohil)
  * update the commit message of patch #3 to explain the expected return
    values of various bpf helpers (Yonghong)

v1: https://lore.kernel.org/bpf/[email protected]/
====================

Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Alexei Starovoitov <[email protected]>
  • Loading branch information
Alexei Starovoitov committed Feb 16, 2024
2 parents e37243b + be66d79 commit 54d46c9
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 9 deletions.
10 changes: 10 additions & 0 deletions arch/x86/include/asm/vsyscall.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include <linux/seqlock.h>
#include <uapi/asm/vsyscall.h>
#include <asm/page_types.h>

#ifdef CONFIG_X86_VSYSCALL_EMULATION
extern void map_vsyscall(void);
Expand All @@ -24,4 +25,13 @@ static inline bool emulate_vsyscall(unsigned long error_code,
}
#endif

/*
* The (legacy) vsyscall page is the long page in the kernel portion
* of the address space that has user-accessible permissions.
*/
static inline bool is_vsyscall_vaddr(unsigned long vaddr)
{
return unlikely((vaddr & PAGE_MASK) == VSYSCALL_ADDR);
}

#endif /* _ASM_X86_VSYSCALL_H */
9 changes: 0 additions & 9 deletions arch/x86/mm/fault.c
Original file line number Diff line number Diff line change
Expand Up @@ -798,15 +798,6 @@ show_signal_msg(struct pt_regs *regs, unsigned long error_code,
show_opcodes(regs, loglvl);
}

/*
* The (legacy) vsyscall page is the long page in the kernel portion
* of the address space that has user-accessible permissions.
*/
static bool is_vsyscall_vaddr(unsigned long vaddr)
{
return unlikely((vaddr & PAGE_MASK) == VSYSCALL_ADDR);
}

static void
__bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
unsigned long address, u32 pkey, int si_code)
Expand Down
10 changes: 10 additions & 0 deletions arch/x86/mm/maccess.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include <linux/uaccess.h>
#include <linux/kernel.h>

#include <asm/vsyscall.h>

#ifdef CONFIG_X86_64
bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
{
Expand All @@ -15,6 +17,14 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
if (vaddr < TASK_SIZE_MAX + PAGE_SIZE)
return false;

/*
* Reading from the vsyscall page may cause an unhandled fault in
* certain cases. Though it is at an address above TASK_SIZE_MAX, it is
* usually considered as a user space address.
*/
if (is_vsyscall_vaddr(vaddr))
return false;

/*
* Allow everything during early boot before 'x86_virt_bits'
* is initialized. Needed for instruction decoding in early
Expand Down
57 changes: 57 additions & 0 deletions tools/testing/selftests/bpf/prog_tests/read_vsyscall.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (C) 2024. Huawei Technologies Co., Ltd */
#include "test_progs.h"
#include "read_vsyscall.skel.h"

#if defined(__x86_64__)
/* For VSYSCALL_ADDR */
#include <asm/vsyscall.h>
#else
/* To prevent build failure on non-x86 arch */
#define VSYSCALL_ADDR 0UL
#endif

struct read_ret_desc {
const char *name;
int ret;
} all_read[] = {
{ .name = "probe_read_kernel", .ret = -ERANGE },
{ .name = "probe_read_kernel_str", .ret = -ERANGE },
{ .name = "probe_read", .ret = -ERANGE },
{ .name = "probe_read_str", .ret = -ERANGE },
{ .name = "probe_read_user", .ret = -EFAULT },
{ .name = "probe_read_user_str", .ret = -EFAULT },
{ .name = "copy_from_user", .ret = -EFAULT },
{ .name = "copy_from_user_task", .ret = -EFAULT },
};

void test_read_vsyscall(void)
{
struct read_vsyscall *skel;
unsigned int i;
int err;

#if !defined(__x86_64__)
test__skip();
return;
#endif
skel = read_vsyscall__open_and_load();
if (!ASSERT_OK_PTR(skel, "read_vsyscall open_load"))
return;

skel->bss->target_pid = getpid();
err = read_vsyscall__attach(skel);
if (!ASSERT_EQ(err, 0, "read_vsyscall attach"))
goto out;

/* userspace may don't have vsyscall page due to LEGACY_VSYSCALL_NONE,
* but it doesn't affect the returned error codes.
*/
skel->bss->user_ptr = (void *)VSYSCALL_ADDR;
usleep(1);

for (i = 0; i < ARRAY_SIZE(all_read); i++)
ASSERT_EQ(skel->bss->read_ret[i], all_read[i].ret, all_read[i].name);
out:
read_vsyscall__destroy(skel);
}
45 changes: 45 additions & 0 deletions tools/testing/selftests/bpf/progs/read_vsyscall.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (C) 2024. Huawei Technologies Co., Ltd */
#include <linux/types.h>
#include <bpf/bpf_helpers.h>

#include "bpf_misc.h"

int target_pid = 0;
void *user_ptr = 0;
int read_ret[8];

char _license[] SEC("license") = "GPL";

SEC("fentry/" SYS_PREFIX "sys_nanosleep")
int do_probe_read(void *ctx)
{
char buf[8];

if ((bpf_get_current_pid_tgid() >> 32) != target_pid)
return 0;

read_ret[0] = bpf_probe_read_kernel(buf, sizeof(buf), user_ptr);
read_ret[1] = bpf_probe_read_kernel_str(buf, sizeof(buf), user_ptr);
read_ret[2] = bpf_probe_read(buf, sizeof(buf), user_ptr);
read_ret[3] = bpf_probe_read_str(buf, sizeof(buf), user_ptr);
read_ret[4] = bpf_probe_read_user(buf, sizeof(buf), user_ptr);
read_ret[5] = bpf_probe_read_user_str(buf, sizeof(buf), user_ptr);

return 0;
}

SEC("fentry.s/" SYS_PREFIX "sys_nanosleep")
int do_copy_from_user(void *ctx)
{
char buf[8];

if ((bpf_get_current_pid_tgid() >> 32) != target_pid)
return 0;

read_ret[6] = bpf_copy_from_user(buf, sizeof(buf), user_ptr);
read_ret[7] = bpf_copy_from_user_task(buf, sizeof(buf), user_ptr,
bpf_get_current_task_btf(), 0);

return 0;
}

0 comments on commit 54d46c9

Please sign in to comment.