From patchwork Fri Apr 17 00:00:06 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jann Horn X-Patchwork-Id: 1271939 X-Patchwork-Delegate: bpf@iogearbox.net Return-Path: X-Original-To: incoming-bpf@patchwork.ozlabs.org Delivered-To: patchwork-incoming-bpf@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org (client-ip=23.128.96.18; helo=vger.kernel.org; envelope-from=bpf-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20161025 header.b=PXCdAK3d; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by ozlabs.org (Postfix) with ESMTP id 493GSG6vdNz9sP7 for ; Fri, 17 Apr 2020 10:00:14 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727906AbgDQAAO (ORCPT ); Thu, 16 Apr 2020 20:00:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41650 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1726578AbgDQAAN (ORCPT ); Thu, 16 Apr 2020 20:00:13 -0400 Received: from mail-vk1-xa49.google.com (mail-vk1-xa49.google.com [IPv6:2607:f8b0:4864:20::a49]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8E860C061A0F for ; Thu, 16 Apr 2020 17:00:13 -0700 (PDT) Received: by mail-vk1-xa49.google.com with SMTP id j68so156935vkj.12 for ; Thu, 16 Apr 2020 17:00:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:message-id:mime-version:subject:from:to:cc; bh=o1ixzjSmDRXRxbpBdm6/pKUbvi6GyqGWdNx6NzDK7xY=; b=PXCdAK3d3tBaw6kqsPb8vcGis/R2zpp+aDNtUJZYEVbxC6NrSt5rJWpNcyGVQkh/KD jP9lLHVBljzVSbiwZ1EWcc+E8XIlV+8gi/K0PVMJ9CDcRvC2SAsHH6vbDYHOrTmvmbZk wayBiSv4AxqUCm/c982HxZmDRbeHjqN8TtlV21Bk5+MIPjXZfeSxbUGmfGU8JhjqZiq0 4JJojRnm3SCXVCF8Msgbxc2Zrz+a2rV0aebyHg+oWZw/1rqjUg0V/iwdWWiHEScWCFXK 9IoEyZOc0VnNherFWKHNpI864ABD2DEBI3vbPYqXEdtwKm97Xnh436XbPY7Ulsz1tqZt NwWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:message-id:mime-version:subject:from:to:cc; bh=o1ixzjSmDRXRxbpBdm6/pKUbvi6GyqGWdNx6NzDK7xY=; b=ZJmhtK8ddQhYIce2Mq22PR4gy54Z49wLUT0bU1B4EUh4LdsWndUBfg7j/etOHhZMll EEKt4BTauTBTsmUwEgcbmaCrKmo0dbNt60tY1QfbVosGy3EQb9t9/PqE6MIi4j5TDbB8 XX8f/KH4Mv0A9Oneq4ZwfmAcLu0bPShyP6ag2jYqgE9D1rI6VuYxFWeH+t9+SZhgsAIm 2WCRvwKW3iqZnZdtlvvZCj4qydYcYfd9bwVcdf645E27DgVqOYGrMJXai2M4mB0ToAVA J8imM7pd9hM1gPJ+10IwDArmo6BdE25NZvGGxBkRlxZ3XlyjzEEQdBNlQwHSj/FibZlQ CFng== X-Gm-Message-State: AGi0PuYWSCMkpkCk21YMTN1MVnoUFrEpsmIcTRWKOGoXxlb4xWZ3uGUk 2nFmIH/Zm9GL6edeHqfu+5Yih7VNo6lfHHNREQLxlxd4eqgupC9erSiL7mTTo84ez1h4SN36lxl aPPzLJ2cqP1Ucar79l3+i2OM/MR6A0aXfdnrNcxIlwHrbzuO4t7QCVw8= X-Google-Smtp-Source: APiQypLGhwVpsiFWV0kz+aHZGYo2KNhREwT7oZoT3nuKhl0+e/KZ16M79NTvjR0IzM62ub0eXcugeeg7Ww== X-Received: by 2002:a1f:9cc8:: with SMTP id f191mr449820vke.68.1587081612594; Thu, 16 Apr 2020 17:00:12 -0700 (PDT) Date: Fri, 17 Apr 2020 02:00:06 +0200 Message-Id: <20200417000007.10734-1-jannh@google.com> Mime-Version: 1.0 X-Mailer: git-send-email 2.26.1.301.g55bc3eb7cb9-goog Subject: [PATCH v2 1/2] bpf: Forbid XADD on spilled pointers for unprivileged users From: Jann Horn To: bpf@vger.kernel.org, Alexei Starovoitov , Daniel Borkmann Cc: Martin KaFai Lau , Song Liu , Yonghong Song , Andrii Nakryiko , John Fastabend , KP Singh , netdev@vger.kernel.org Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org When check_xadd() verifies an XADD operation on a pointer to a stack slot containing a spilled pointer, check_stack_read() verifies that the read, which is part of XADD, is valid. However, since the placeholder value -1 is passed as `value_regno`, check_stack_read() can only return a binary decision and can't return the type of the value that was read. The intent here is to verify whether the value read from the stack slot may be used as a SCALAR_VALUE; but since check_stack_read() doesn't check the type, and the type information is lost when check_stack_read() returns, this is not enforced, and a malicious user can abuse XADD to leak spilled kernel pointers. Fix it by letting check_stack_read() verify that the value is usable as a SCALAR_VALUE if no type information is passed to the caller. To be able to use __is_pointer_value() in check_stack_read(), move it up. Fix up the expected unprivileged error message for a BPF selftest that, until now, assumed that unprivileged users can use XADD on stack-spilled pointers. This also gives us a test for the behavior introduced in this patch for free. In theory, this could also be fixed by forbidding XADD on stack spills entirely, since XADD is a locked operation (for operations on memory with concurrency) and there can't be any concurrency on the BPF stack; but Alexei has said that he wants to keep XADD on stack slots working to avoid changes to the test suite [1]. The following BPF program demonstrates how to leak a BPF map pointer as an unprivileged user using this bug: // r7 = map_pointer BPF_LD_MAP_FD(BPF_REG_7, small_map), // r8 = launder(map_pointer) BPF_STX_MEM(BPF_DW, BPF_REG_FP, BPF_REG_7, -8), BPF_MOV64_IMM(BPF_REG_1, 0), ((struct bpf_insn) { .code = BPF_STX | BPF_DW | BPF_XADD, .dst_reg = BPF_REG_FP, .src_reg = BPF_REG_1, .off = -8 }), BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_FP, -8), // store r8 into map BPF_MOV64_REG(BPF_REG_ARG1, BPF_REG_7), BPF_MOV64_REG(BPF_REG_ARG2, BPF_REG_FP), BPF_ALU64_IMM(BPF_ADD, BPF_REG_ARG2, -4), BPF_ST_MEM(BPF_W, BPF_REG_ARG2, 0, 0), BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1), BPF_EXIT_INSN(), BPF_STX_MEM(BPF_DW, BPF_REG_0, BPF_REG_8, 0), BPF_MOV64_IMM(BPF_REG_0, 0), BPF_EXIT_INSN() [1] https://lore.kernel.org/bpf/20200416211116.qxqcza5vo2ddnkdq@ast-mbp.dhcp.thefacebook.com/ Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)") Signed-off-by: Jann Horn --- kernel/bpf/verifier.c | 28 +++++++++++++------ .../bpf/verifier/value_illegal_alu.c | 1 + 2 files changed, 20 insertions(+), 9 deletions(-) base-commit: edadedf1c5b4e4404192a0a4c3c0c05e3b7672ab diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 38cfcf701eeb7..9e92d3d5ffd17 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2118,6 +2118,15 @@ static bool register_is_const(struct bpf_reg_state *reg) return reg->type == SCALAR_VALUE && tnum_is_const(reg->var_off); } +static bool __is_pointer_value(bool allow_ptr_leaks, + const struct bpf_reg_state *reg) +{ + if (allow_ptr_leaks) + return false; + + return reg->type != SCALAR_VALUE; +} + static void save_register_state(struct bpf_func_state *state, int spi, struct bpf_reg_state *reg) { @@ -2308,6 +2317,16 @@ static int check_stack_read(struct bpf_verifier_env *env, * which resets stack/reg liveness for state transitions */ state->regs[value_regno].live |= REG_LIVE_WRITTEN; + } else if (__is_pointer_value(env->allow_ptr_leaks, reg)) { + /* If value_regno==-1, the caller is asking us whether + * it is acceptable to use this value as a SCALAR_VALUE + * (e.g. for XADD). + * We must not allow unprivileged callers to do that + * with spilled pointers. + */ + verbose(env, "leaking pointer from stack off %d\n", + off); + return -EACCES; } mark_reg_read(env, reg, reg->parent, REG_LIVE_READ64); } else { @@ -2673,15 +2692,6 @@ static int check_sock_access(struct bpf_verifier_env *env, int insn_idx, return -EACCES; } -static bool __is_pointer_value(bool allow_ptr_leaks, - const struct bpf_reg_state *reg) -{ - if (allow_ptr_leaks) - return false; - - return reg->type != SCALAR_VALUE; -} - static struct bpf_reg_state *reg_state(struct bpf_verifier_env *env, int regno) { return cur_regs(env) + regno; diff --git a/tools/testing/selftests/bpf/verifier/value_illegal_alu.c b/tools/testing/selftests/bpf/verifier/value_illegal_alu.c index 7f6c232cd8423..ed1c2cea1dea6 100644 --- a/tools/testing/selftests/bpf/verifier/value_illegal_alu.c +++ b/tools/testing/selftests/bpf/verifier/value_illegal_alu.c @@ -88,6 +88,7 @@ BPF_EXIT_INSN(), }, .fixup_map_hash_48b = { 3 }, + .errstr_unpriv = "leaking pointer from stack off -8", .errstr = "R0 invalid mem access 'inv'", .result = REJECT, .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,