From patchwork Mon Oct 16 15:45:52 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Kicinski X-Patchwork-Id: 826400 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netdev-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=netronome-com.20150623.gappssmtp.com header.i=@netronome-com.20150623.gappssmtp.com header.b="WD7aV2Ya"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3yG2jl3jQDz9t32 for ; Tue, 17 Oct 2017 02:46:19 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753742AbdJPPqR (ORCPT ); Mon, 16 Oct 2017 11:46:17 -0400 Received: from mail-pg0-f42.google.com ([74.125.83.42]:47194 "EHLO mail-pg0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752415AbdJPPqP (ORCPT ); Mon, 16 Oct 2017 11:46:15 -0400 Received: by mail-pg0-f42.google.com with SMTP id r25so7126580pgn.4 for ; Mon, 16 Oct 2017 08:46:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netronome-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id; bh=TTX+Tf7f9o5yKedhCkWxYXuoBun9emaX/IEi3Z+BKEY=; b=WD7aV2YaPt0+78GF4IX9EgnRZtIOo77OwPgFsJAIZeb08s0RIdsu0ZIHGNRlU9Tqwp OCf7trngSCggkKelIYbAll3Erd3mRU145x4uQc1jL9ZC/4pLBMG0sIIyiyftkIqMRCc3 cy+FW4aw33b9aNN167m+PwuQniYpYmBBe97DESto6BwgLXG4M7p171l1v7JnAfrFPrDf N20MiAFBi5K/K/T68OEl0X/nj5ge5WBw5BYj50X2KagYvls7dCG1cqxArR/pwBnbYIz1 mqllWy4WwSu4EXBPq3Heb8VHTJrzqotzRUJ4gwitLol+Gyp+zVAblkXGVGhFns5+Isvh XAuA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=TTX+Tf7f9o5yKedhCkWxYXuoBun9emaX/IEi3Z+BKEY=; b=ANiDkPMpDNQd/PvGNKsuzoUNlfl3+CJgmrHNdfy/hMHRS0vSnHwcDbPEi1MC3AqO8G H3aJ/kAqOuF4iLTJq3+CUhN3HEtFsPY2KYaXG/w8Owk6mFIXxuBeOSy+13Lana7uCbA5 eGICn3kGnMx/TAbIjubYuKgzrVKAmwIr7CSGc8mXcdmhInUIWEHWmISUYhT3yhDPFrWm 0TywKK4r5H2QxGUyaukT8rk6+MDabXIVKaxVp5HgNecEetYsoV9nn3KtuWYJ/E5sJhlt xrgwk50cIQiuNLsNSHBILQqlbImaNH0ffNAdl1ugQch0/0kt6cYPnX6TqG2KZJLQN2Si 5QBQ== X-Gm-Message-State: AMCzsaXNe1/BYMgVftMpQhbjznqW1VacGiEt4QbFL+GIh8q+WDPWe3Su NsHFpl67YGNu+dbpynJZYbhz9q+R X-Google-Smtp-Source: AOwi7QBHu616DXGiIrEh2WlsrnQmXVsu8c/S+HVVaOt30T9XAwdzvvhWGzfNaI8a5dEuBRF8vf/GUA== X-Received: by 10.98.161.24 with SMTP id b24mr9309675pff.297.1508168774933; Mon, 16 Oct 2017 08:46:14 -0700 (PDT) Received: from jkicinski-Precision-T1700.netronome.com ([75.53.12.129]) by smtp.gmail.com with ESMTPSA id z126sm14976198pfz.70.2017.10.16.08.46.13 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 16 Oct 2017 08:46:14 -0700 (PDT) From: Jakub Kicinski To: netdev@vger.kernel.org Cc: oss-drivers@netronome.com, alexei.starovoitov@gmail.com, daniel@iogearbox.net, ecree@solarflare.com, Jakub Kicinski Subject: [PATCH net] bpf: disallow arithmetic operations on context pointer Date: Mon, 16 Oct 2017 08:45:52 -0700 Message-Id: <20171016154552.30640-1-jakub.kicinski@netronome.com> X-Mailer: git-send-email 2.14.1 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Commit f1174f77b50c ("bpf/verifier: rework value tracking") removed the crafty selection of which pointer types are allowed to be modified. This is OK for most pointer types since adjust_ptr_min_max_vals() will catch operations on immutable pointers. One exception is PTR_TO_CTX which is now allowed to be offseted freely. The intent of aforementioned commit was to allow context access via modified registers. The offset passed to ->is_valid_access() verifier callback has been adjusted by the value of the variable offset. What is missing, however, is taking the variable offset into account when the context register is used. Or in terms of the code adding the offset to the value passed to the ->convert_ctx_access() callback. This leads to the following eBPF user code: r1 += 68 r0 = *(u32 *)(r1 + 8) exit being translated to this in kernel space: 0: (07) r1 += 68 1: (61) r0 = *(u32 *)(r1 +180) 2: (95) exit Offset 8 is corresponding to 180 in the kernel, but offset 76 is valid too. Verifier will "accept" access to offset 68+8=76 but then "convert" access to offset 8 as 180. Effective access to offset 248 is beyond the kernel context. (This is a __sk_buff example on a debug-heavy kernel - packet mark is 8 -> 180, 76 would be data.) Dereferencing the modified context pointer is not as easy as dereferencing other types, because we have to translate the access to reading a field in kernel structures which is usually at a different offset and often of a different size. To allow modifying the pointer we would have to make sure that given eBPF instruction will always access the same field or the fields accessed are "compatible" in terms of offset and size... Disallow dereferencing modified context pointers and add to selftests the test case described here. Fixes: f1174f77b50c ("bpf/verifier: rework value tracking") Signed-off-by: Jakub Kicinski --- Dave, a merge note - in net-next this will need env to be passed to verbose(). kernel/bpf/verifier.c | 8 ++++++-- tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 8b8d6ba39e23..8499759d0c7a 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1116,7 +1116,12 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn /* ctx accesses must be at a fixed offset, so that we can * determine what type of data were returned. */ - if (!tnum_is_const(reg->var_off)) { + if (reg->off) { + verbose("derefence of modified ctx ptr R%d off=%d+%d, ctx+const is allowed, ctx+const+const is not\n", + regno, reg->off, off - reg->off); + return -EACCES; + } + if (!tnum_is_const(reg->var_off) || reg->var_off.value) { char tn_buf[48]; tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off); @@ -1124,7 +1129,6 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn tn_buf, off, size); return -EACCES; } - off += reg->var_off.value; err = check_ctx_access(env, insn_idx, off, size, t, ®_type); if (!err && t == BPF_READ && value_regno >= 0) { /* ctx access returns either a scalar, or a diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 26f3250bdcd2..d41c77e7b39b 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -6645,6 +6645,20 @@ static struct bpf_test tests[] = { .errstr = "BPF_END uses reserved fields", .result = REJECT, }, + { + "arithmetic ops make PTR_TO_CTX unusable", + .insns = { + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, + offsetof(struct __sk_buff, data) - + offsetof(struct __sk_buff, mark)), + BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1, + offsetof(struct __sk_buff, mark)), + BPF_EXIT_INSN(), + }, + .errstr = "derefence of modified ctx ptr R1 off=68+8, ctx+const is allowed, ctx+const+const is not", + .result = REJECT, + .prog_type = BPF_PROG_TYPE_SCHED_CLS, + }, }; static int probe_filter_length(const struct bpf_insn *fp)