Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Roberto Sassu <roberto.sassu@huaweicloud.com>
To:     ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        martin.lau@linux.dev, song@kernel.org, yhs@fb.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, revest@chromium.org,
        jackmanb@chromium.org, mykolal@fb.com, paul@paul-moore.com,
        jmorris@namei.org, serge@hallyn.com, shuah@kernel.org
Cc:     bpf@vger.kernel.org, linux-security-module@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        Roberto Sassu <roberto.sassu@huawei.com>
Subject: [RFC][PATCH v2 0/7] bpf-lsm: Check return values of security modules
Date:   Wed,  7 Dec 2022 18:24:27 +0100
Message-Id: <20221207172434.435893-1-roberto.sassu@huaweicloud.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Roberto Sassu <roberto.sassu@huawei.com>

Notes:
- This patch set addresses the kernel panic described below, and not the
  more broad issue of accessing kernel objects whose pointer is passed
  as parameter by LSM hooks
- Alternative approaches trying to limit return values at run-time either
  in the security subsystem or in the eBPF JIT are not preferred by the
  respective maintainers
- Although all eBPF selftests have been verified to pass, it still might
  be cumbersome to have an eBPF program being accepted by the eBPF
  verifier (e.g. ANDing negative numbers causes existing bounds to be lost)
- The patch to store whether a register state changed due to an ALU64 or an
  ALU32 operation might not be correct/complete, a review by eBPF
  maintainers would be needed
- This patch set requires "lsm: make security_socket_getpeersec_stream()
  sockptr_t safe", in lsm/next
- The modification of the LSM infrastructure to define allowed return
  values for the LSM hooks could be replaced with an eBPF-only fix, with
  the drawback of having to update the information manually each time a
  new hook is added; allowing zero or negative values by default could be
  reasonable, but there are already exceptions of LSM hooks accepting 0 or
  1 (ismaclabel)
- The patches to fix the LSM infrastructure documentation are separated
  from this patch set and available here:
  https://lore.kernel.org/linux-security-module/20221128144240.210110-1-roberto.sassu@huaweicloud.com/

BPF LSM defines attachment points to allows security modules (eBPF programs
with type LSM) to provide their implementation of the desired LSM hooks.

Unfortunately, BPF LSM does not restrict which values security modules can
return (for non-void LSM hooks). If they put arbitrary values instead of
those stated in include/linux/lsm_hooks.h, they could cause big troubles.

For example, this simple eBPF program:

SEC("lsm/inode_permission")
int BPF_PROG(test_int_hook, struct inode *inode, int mask)
{
	return 1;
}

causes the following kernel panic:

[  181.130807] BUG: kernel NULL pointer dereference, address: 0000000000000079
[  181.131478] #PF: supervisor read access in kernel mode
[  181.131942] #PF: error_code(0x0000) - not-present page
[  181.132407] PGD 0 P4D 0 
[  181.132650] Oops: 0000 [#1] PREEMPT SMP NOPTI
[  181.133054] CPU: 5 PID: 857 Comm: systemd-oomd Tainted: G           OE      6.1.0-rc7+ #530
[  181.133806] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[  181.134601] RIP: 0010:do_sys_openat2+0x235/0x300

[...]

[  181.136682] RSP: 0018:ffffc90001557ee0 EFLAGS: 00010203
[  181.137154] RAX: 0000000000000001 RBX: ffffc90001557f20 RCX: ffff888112003380
[  181.137790] RDX: 0000000000000000 RSI: ffffffff8280b026 RDI: ffffc90001557e28
[  181.138432] RBP: 0000000000000001 R08: 0000000000000001 R09: 0000000000000000
[  181.139081] R10: ffffffff835097dc R11: 0000000000000000 R12: ffff888106118000
[  181.139717] R13: 000000000000000c R14: 0000000000000000 R15: 0000000000000000
[  181.140149] FS:  00007fa6ceb0bb40(0000) GS:ffff88846fb40000(0000) knlGS:0000000000000000
[  181.140556] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  181.140865] CR2: 0000000000000079 CR3: 0000000135c50000 CR4: 0000000000350ee0
[  181.141239] Call Trace:
[  181.141373]  <TASK>
[  181.141495]  do_sys_open+0x34/0x60
[  181.141678]  do_syscall_64+0x3b/0x90
[  181.141875]  entry_SYSCALL_64_after_hwframe+0x63/0xcd

Avoid this situation by statically analyzing the eBPF programs attaching to
LSM hooks, and ensure that their return values are compatible with the LSM
infrastructure conventions.

First, add a preliminary patch (patch 1) to fix a small code duplication
issue.

Extend the eBPF verifier to let BPF LSM determine whether it should check
estimated 64 bit values or the 32 bit ones (patch 2). Also, extend the LSM
infrastructure to record more precisely the allowed return values depending
on the documentation found in include/linux/lsm_hooks.h (patch 3). Add the
LSM_RET_NEG, LSM_RET_ZERO, LSM_RET_ONE, LSM_RET_GT_ONE flags to an LSM hook
if that hook allows respectively > 0, 0, 1, > 1 return values.

Then, extend BPF LSM to verify that return values, estimated by the
verifier by analyzing the eBPF program, fall in the allowed intervals found
from the return value flags of the LSM hook being attached to (patch 4).

Finally, add new tests to ensure that the verifier enforces return values
correctly (patch 5), and slightly modify existing tests to make them follow
the LSM infrastructure conventions (patches 6-7) and are accepted by the
verifier.

Changelog:

v1:
- Complete the documentation of return values in lsm_hooks.h
- Introduce return value flags in the LSM infrastructure
- Use those flags instead of the scattered logic (suggested by KP)
- Expose a single verification function to the verifier (suggested by KP)
- Add new patch to remove duplicated function definition
- Add new patch to let BPF LSM determine the appropriate register values
  to use

Roberto Sassu (7):
  bpf: Remove superfluous btf_id_set_contains() declaration
  bpf: Mark ALU32 operations in bpf_reg_state structure
  lsm: Redefine LSM_HOOK() macro to add return value flags as argument
  bpf-lsm: Enforce return value limitations on security modules
  selftests/bpf: Check if return values of LSM programs are allowed
  selftests/bpf: Prevent positive ret values in test_lsm and
    verify_pkcs7_sig
  selftests/bpf: Change return value in test_libbpf_get_fd_by_id_opts.c

 include/linux/bpf.h                           |   1 -
 include/linux/bpf_lsm.h                       |  11 +-
 include/linux/bpf_verifier.h                  |   1 +
 include/linux/lsm_hook_defs.h                 | 780 ++++++++++--------
 include/linux/lsm_hooks.h                     |   9 +-
 kernel/bpf/bpf_lsm.c                          |  81 +-
 kernel/bpf/verifier.c                         |  17 +-
 security/bpf/hooks.c                          |   2 +-
 security/security.c                           |   4 +-
 tools/testing/selftests/bpf/progs/lsm.c       |   4 +
 .../bpf/progs/test_libbpf_get_fd_by_id_opts.c |   7 +-
 .../bpf/progs/test_verify_pkcs7_sig.c         |  11 +-
 .../testing/selftests/bpf/verifier/lsm_ret.c  | 148 ++++
 13 files changed, 729 insertions(+), 347 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/verifier/lsm_ret.c

-- 
2.25.1