From patchwork Fri Dec 6 16:10:51 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13897357 X-Patchwork-Delegate: bpf@iogearbox.net Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6681E20D50E for ; Fri, 6 Dec 2024 16:10:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.65 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733501461; cv=none; b=LDwO62D51SxC0RKSzgb8UG9MKXP3+Eg3e/DR98jyQjJuQ+Zllu5vySK2X3ju3Y4akENMt15OWZpvWr8ar+lHyGu25CaBhYQGBzSZSXJ9hiX8zZ8lMAfqBWvuOwAmrkz1FVOv6EUyaIDqXDUp+ASL4h5vQiwYKNHSHTZPcF1xbfU= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733501461; c=relaxed/simple; bh=IV4WwrNiULRwAafUxmJqzO84gg9SB0SzgoMo6BwtuIo=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=pC1T5dYV1sQUTtXRUfSt69dUv2x/Y9Dj68ZcxwWQjltDVrhDJZ/myr5pkGLjm/IrVJ7hoJxHyVGuD0Cs+8NBS9gRd7G6Y2zStJC3fR0gGUbTBKrBcyDPeBBPow8Y35X0lcF/qjvwIs04/o4oxnfSoCGBqIodc18dqY9YNwVJJ20= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=Sdkz9xZe; arc=none smtp.client-ip=209.85.128.65 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Sdkz9xZe" Received: by mail-wm1-f65.google.com with SMTP id 5b1f17b1804b1-434aabd688fso14988475e9.3 for ; Fri, 06 Dec 2024 08:10:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1733501457; x=1734106257; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=3QCSBnVm04PkHslLUEhSA4GXN/6Jz6KNydKP3UpMZQM=; b=Sdkz9xZeROJDdaDBzjA9JdziOGY/k+83Bd2Kle3WxKokqfiVJ+x+CtNR/kk03YWx5T +RZYV2gO90Be3S0XmFxcgWoQa4usiuHR1ulU71ewVMggu6sHYL90UKAW36WdwFpe0zSl 1Y/DIKcrl7bk7H6dJXW0wTkQGfkWRbKaIg2Hol9JTb9mnLzPU1sUPm/V61BngbTtTLUu kWxtSm6osDn4KZdo0Czo0pxfkqUt4vAtkuGiMQamDdXYmVPdutqv9fwyHTzum8Ex3tfa ugocnoUXrxMm8LH3uzpFI6Su8qZEL9VKbPjnAjBN+Cy/4nQBzg9iUBeWlMiktf1bk1sK 9IbA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733501457; x=1734106257; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=3QCSBnVm04PkHslLUEhSA4GXN/6Jz6KNydKP3UpMZQM=; b=GtB8oK5eS1cd9SHyIybhiSXDparplfntLsT/ls25jRRI/Lw+ArX4rk1kQvnBYhwkCj md23ULUdrWK1lfVIrLYJAwzH2K//XfTXhZwWWIIhqUVB0br6XeMPHpDlyDlZtnIBri3x TT1V0cn+Z4VvnfFoGgv+JIh1rzYg/vPTH62ZITWlMH8i1M1yg8/NWSmFMFEQbZ2Og0Dq yD0kxSemI9RSh4KiO8f1GDUfa2tCN+aw+N+xnAxbwr5aEq0sfkQ0/Wl4SHY2IT8S8gP3 JUnkDAiAfnCcWtBvrqI/iZs3c0EFFy2WMoZu3NRlzZMo9+BFn/OrZcmntLZdVFf2olJu wnrw== X-Gm-Message-State: AOJu0YzP41roYo8SCoM63fstNYIivKkYCuXiQAgnw+hBDprgNTZ0Zys7 HSr8nQc/56aBTt+sbgN9TFuUI4EUPHBAiidLzZpmgHIKKngvAiJC4C6zY3TOzvU= X-Gm-Gg: ASbGnctbbde42De3R4RB23ivFXmlZVvXc4QIXFF/PThISkAWn+NovvGgCp1VAK7Fmqr H4DuZdlUBi0x6qcrUzbpS01gXQaGYxYsOXrlMiQWMORA2+rheY5Xv7bPJoGxxgam0iBoj2MdXcg AQoM+X4g/93KtHDxIg12kGAPWeLlmS4ugXutnxssv0X5NR02byrbEUW9gtRm+6K8ILpsCyj1gxK J8Q0He2BfAbk9znZoftmUfj/E+6RH4xRZlKYQhdmdB5bBbGIiV0/uP1PzJpFEe2rKVtB0YE46HH X-Google-Smtp-Source: AGHT+IH2EJmzcqZVEAJs8KUH/YfLMsZtdxJ/9LLtNONmuNbDyDLa3dX8yUy6JM14O3RvfBv7CcdNiA== X-Received: by 2002:a5d:47a1:0:b0:385:ec89:2f07 with SMTP id ffacd0b85a97d-3862b38ee31mr2963517f8f.32.1733501457071; Fri, 06 Dec 2024 08:10:57 -0800 (PST) Received: from localhost (fwdproxy-cln-007.fbsv.net. [2a03:2880:31ff:7::face:b00c]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-434d52c12a4sm98835635e9.30.2024.12.06.08.10.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 06 Dec 2024 08:10:55 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: kkd@meta.com, Manu Bretelle , Eduard Zingerman , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , kernel-team@fb.com Subject: [PATCH bpf v3 1/3] bpf: Suppress warning for non-zero off raw_tp arg NULL check Date: Fri, 6 Dec 2024 08:10:51 -0800 Message-ID: <20241206161053.809580-2-memxor@gmail.com> X-Mailer: git-send-email 2.43.5 In-Reply-To: <20241206161053.809580-1-memxor@gmail.com> References: <20241206161053.809580-1-memxor@gmail.com> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=5690; h=from:subject; bh=IV4WwrNiULRwAafUxmJqzO84gg9SB0SzgoMo6BwtuIo=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBnUyEsZEvhHrWlFA7R4CIKwPl3JsSKUHl5RWWRnefz yL7w3KCJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCZ1MhLAAKCRBM4MiGSL8RyvBjEA ChlnpjgfqlHbB7/TUvwidMU537roUQfafTURY2SozCoTdqZi9LgUtooox7tlg0ciV4fx1yTp/D0wqJ FPhV3KG1IaVTNkrezYgaIeaGCq/1rxllzR4pG/hhb97yC2pZHmwGDJAimsfYYe0TKwMBPJ3W8b5htH obig7smNNCDhlRh3fQvh4p2UdjBZE7ARqIliA+ERC/ReagKU+EEMAwdoJPRnIeuKVuykkUjbt4Y77q gg9kKdlVlnBKouQSaNhv27IeD+DXIdyTpEEZdEaj8TOLNY6JnV2/7y4TgJqe3OPxMLxaRXhv9YdZel sAgxhg5trOpdG3IFgt3o59PKoZGIals4I8HISmmi/3z2XtBpVCURDt6j390Uz9QmyMjjZBleCjcDRC mBHb34hTgwREk0u6F0QvC1N8G4lvqwqSjNYvTA4HMGDfRa2/AKyNOl5OpLVjIxhcjFCm7Pz28PmA8b tJ318gSb2+gSuE0c1nFWSqcyMWv7sk5Ld35nuXZ0bDwg0aOHjFiYOq7xpv47auur0eGRwx3A7yqfdR IgGeoejER4In9AztAhsOsZ2k/GuJCRuZiuNVhk3NODkSbvyr+zAVjgb/K0rCeqv91owK1KKbpCjv7a 6LXrbkopRRCZK3/xkKoVmhGx4NKn21POMZo/geg8Tx4jb/hh7WMl0XTFMNQQ== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA X-Patchwork-Delegate: bpf@iogearbox.net The fixed commit began marking raw_tp arguments as PTR_MAYBE_NULL to avoid dead code elimination in the verifier, since raw_tp arguments may actually be NULL at runtime. However, to preserve compatibility, it simulated the raw_tp accesses as if the NULL marking was not present. One of the behaviors permitted by this simulation is offset modification for NULL pointers. Typically, this pattern is rejected by the verifier, and users make workarounds to prevent the compiler from producing such patterns. However, now that it is allowed, when the compiler emits such code, the offset modification is allowed and a PTR_MAYBE_NULL raw_tp arg with non-zero off can be formed. The failing example program had the following pseudo-code: r0 = 1024; r1 = ...; // r1 = trusted_or_null_(id=1) r3 = r1; // r3 = trusted_or_null_(id=1) r1 = trusted_or_null_(id=1) r3 += r0; // r3 = trusted_or_null_(id=1, off=1024) if r1 == 0 goto pc+X; At this point, while mark_ptr_or_null_reg will see PTR_MAYBE_NULL and off == 0 for r1, it will notice non-zero off for r3, and the WARN_ON_ONCE will fire, as the condition checks excluding register types do not include raw_tp argument type. This is a pattern produced by LLVM, therefore it is hard to suppress it everywhere in BPF programs. The right "generic" fix for this issue in general, will be permitting offset modification for PTR_MAYBE_NULL pointers everywhere, and enforcing that the instruction operand of a conditional jump has the offset as zero. It's other copies may still have non-zero offset, and that is fine. But this is more involved and will take longer to integrate. If a zero offset pointer is NULL checked, all copies can be marked non-NULL, while checking non-zero offset PTR_MAYBE_NULL is a no-op. For now, only make this change for raw_tp arguments, and table the generic fix for later. Dereferencing such pointers will still work as the fixed commit allowed it for raw_tp args. Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL") Reported-by: Manu Bretelle Acked-by: Eduard Zingerman Signed-off-by: Kumar Kartikeya Dwivedi --- kernel/bpf/verifier.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 2fd35465d650..82f40d63ad7b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -15340,7 +15340,8 @@ static int reg_set_min_max(struct bpf_verifier_env *env, return err; } -static void mark_ptr_or_null_reg(struct bpf_func_state *state, +static void mark_ptr_or_null_reg(struct bpf_verifier_env *env, + struct bpf_func_state *state, struct bpf_reg_state *reg, u32 id, bool is_null) { @@ -15357,7 +15358,9 @@ static void mark_ptr_or_null_reg(struct bpf_func_state *state, */ if (WARN_ON_ONCE(reg->smin_value || reg->smax_value || !tnum_equals_const(reg->var_off, 0))) return; - if (!(type_is_ptr_alloc_obj(reg->type) || type_is_non_owning_ref(reg->type)) && + if (!type_is_ptr_alloc_obj(reg->type) && + !type_is_non_owning_ref(reg->type) && + !mask_raw_tp_reg_cond(env, reg) && WARN_ON_ONCE(reg->off)) return; @@ -15390,11 +15393,12 @@ static void mark_ptr_or_null_reg(struct bpf_func_state *state, /* The logic is similar to find_good_pkt_pointers(), both could eventually * be folded together at some point. */ -static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno, +static void mark_ptr_or_null_regs(struct bpf_verifier_env *env, + struct bpf_verifier_state *vstate, u32 regno, bool is_null) { struct bpf_func_state *state = vstate->frame[vstate->curframe]; - struct bpf_reg_state *regs = state->regs, *reg; + struct bpf_reg_state *regs = state->regs, *reg = ®s[regno]; u32 ref_obj_id = regs[regno].ref_obj_id; u32 id = regs[regno].id; @@ -15405,8 +15409,28 @@ static void mark_ptr_or_null_regs(struct bpf_verifier_state *vstate, u32 regno, */ WARN_ON_ONCE(release_reference_state(state, id)); + /* For raw_tp args, compiler can produce code of the following + * pattern: + * r3 = r1; // r1 = trusted_or_null_(id=1) r3 = trusted_or_null_(id=1) + * r3 += 8; // r3 = trusted_or_null_(id=1,off=8) + * if r1 == 0 goto pc+N; // r1 = trusted_(id=1) + * + * But we musn't remove the or_null mark from r3, as it won't be + * NULL. + * + * Only do unmarking of everything sharing id if operand of NULL check + * has off = 0. + */ + if (mask_raw_tp_reg_cond(env, reg) && reg->off) { + /* We don't reset reg->id back to 0, as it's unexpected + * when PTR_MAYBE_NULL is set. Simply avoid performing + * a walk for other registers with the same id. + */ + return; + } + bpf_for_each_reg_in_vstate(vstate, state, reg, ({ - mark_ptr_or_null_reg(state, reg, id, is_null); + mark_ptr_or_null_reg(env, state, reg, id, is_null); })); } @@ -15832,9 +15856,9 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env, /* Mark all identical registers in each branch as either * safe or unknown depending R == 0 or R != 0 conditional. */ - mark_ptr_or_null_regs(this_branch, insn->dst_reg, + mark_ptr_or_null_regs(env, this_branch, insn->dst_reg, opcode == BPF_JNE); - mark_ptr_or_null_regs(other_branch, insn->dst_reg, + mark_ptr_or_null_regs(env, other_branch, insn->dst_reg, opcode == BPF_JEQ); } else if (!try_match_pkt_pointers(insn, dst_reg, ®s[insn->src_reg], this_branch, other_branch) && From patchwork Fri Dec 6 16:10:52 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13897358 X-Patchwork-Delegate: bpf@iogearbox.net Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id AAC76198E86 for ; Fri, 6 Dec 2024 16:11:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.66 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733501462; cv=none; b=NVkI2xyo/Ckm3RTJgHXU9f+WqiFTgwj7sTMLqL4/WYvqBBBiTh5SBVqj2DezvePAcnQSE42uWL7bgM27iWnR7W8PN0D2C30uc04gBJ7/9kuRdkXchpF+T57e+WH3b05QULSZ/vLmPX/uFzT2IoYlqtAYdbT41L04CJdAXOwf5uM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733501462; c=relaxed/simple; bh=6SYnAZ+Yniywq17NYFLuBtuw5RJyC8ixM2EHSWHfS0k=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=iNun9rNbO8dNcsQb2Pa6X+M53b0HXppareMPqnuBVx9V5WJIROrFsSc87IVV6l3NXRh11wEuRIhkopepOOtbjDRB+oRbOb4Nh5t+OBXUix4Yj4vEc4BEetQJP00SOcpyyyfD+cz6/ovuNVo2n2hWsxEtc13u/1QdMyP66y3T5zA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=H4ZulB1D; arc=none smtp.client-ip=209.85.128.66 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="H4ZulB1D" Received: by mail-wm1-f66.google.com with SMTP id 5b1f17b1804b1-434a736518eso25678655e9.1 for ; Fri, 06 Dec 2024 08:11:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1733501458; x=1734106258; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=ozlORsyVdl733TljK6jsEobVUX/kPQQUfwmQFnC3BSI=; b=H4ZulB1DlO/2j3hTHLe7SesPPeTmJXGACyFSNwbEMiSXdAP1OSdxUrMkTl6PAB0ipG 7TJIQYQWAoY3B/n2nZH95/SGeRiRk4FhAWKGSPUxmZoDa/pfphK8biNjRCNjremmsDaq //sZ0/zCHrLJawgZB94v9txa5k7Qg1I9FkIEp6GEhCU9MvIefM1u8/Z5PMRyxk1dzMGw Xyf9id2sG9XpmUAuSwm7lHYLRQTrvI05lxOwdjJPXAF8P8pTn8gg1rVAood5Qx6sDRbT tzIzStWdJeIDyD/1VNgOZ0IFbH5ec+2YM9Y+143O0WV4+EIMxe4PPeUMfVsnPLYQiJo4 r8yg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733501458; x=1734106258; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ozlORsyVdl733TljK6jsEobVUX/kPQQUfwmQFnC3BSI=; b=sp+fkX9KXRxd7Q+ANlUZhHtM6mFNUY5PHSEZnCjy3bnkslU+zr8FU5i3idtKrZg/VC 0a2M5G5h1eoN3hOBYwTA46TAcHv2k7doqcMfh5q6bbs+jm92DbZ+omGbmXIQK08RPqp/ dpAYJZdOYyE/LOjwsz59SrF/IsNyj+XP2ZMOxcMDlzrSDm7GkvPvi0MUt7hAvMwnUOLQ 9mX3MlhknsEcWMQWOhB++Aa91eWGHyYmULDnCe5SKZn/A7zF9v+fBNfN+FFMuMuFMMu+ LhTt0mdxFt7rDYZkmV3ZB58JVJbeYrnwfMt+B1w4TDuppLKwaQhcFzXewk3OH/6zvcF2 8mEA== X-Gm-Message-State: AOJu0YxhQyul3FuSnYQ35qqGMyNEFKZMRpDzYg7sCtOHPlHbHeG4Foho 6+nDG1KgR5xO1cDliZhlxslL12k7uUNR+ZywLmpbs7Y6cQRUSoxVs2f9QzRGEpo= X-Gm-Gg: ASbGnctVbFkuZX9D08xClj2zwxhqKrVnKhy3aP2URv+BedMYjUqwoRlHYlKD92WcpLg vGReDsYaztisIDWrPydxKGqz/YvoPABQDNCAFwQq0dgZx58tFpM9DeOqKeHCMV9b3u9fXivWy8i kXttPgq3K6lXTZ0FWNViTXe66IkSQJctnSZswlQCRFA3GOctwIMLkBQuevlqXDfUCTgIk26bKCR gjY4q8Pmt0Iop/3wFFzorCmmSoQ657PqAk52AWyldodY9zZfODmNU0KCXrxePcd2Kf0eCjRKRft dA== X-Google-Smtp-Source: AGHT+IF7gILd7UfSk0gj5vCm3CCG9iFPT5eDU9ZPkrVooR1CJeQ2zvbPuBqYRWGmfb61ysq5xt+EmQ== X-Received: by 2002:a05:600c:1d85:b0:434:a955:ede with SMTP id 5b1f17b1804b1-434ddeb8d9dmr38503455e9.14.1733501458563; Fri, 06 Dec 2024 08:10:58 -0800 (PST) Received: from localhost (fwdproxy-cln-031.fbsv.net. [2a03:2880:31ff:1f::face:b00c]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-434da0d25d6sm61264655e9.6.2024.12.06.08.10.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 06 Dec 2024 08:10:57 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: kkd@meta.com, Manu Bretelle , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , Eduard Zingerman , kernel-team@fb.com Subject: [PATCH bpf v3 2/3] bpf: Do not mark NULL-checked raw_tp arg as scalar Date: Fri, 6 Dec 2024 08:10:52 -0800 Message-ID: <20241206161053.809580-3-memxor@gmail.com> X-Mailer: git-send-email 2.43.5 In-Reply-To: <20241206161053.809580-1-memxor@gmail.com> References: <20241206161053.809580-1-memxor@gmail.com> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=2888; h=from:subject; bh=6SYnAZ+Yniywq17NYFLuBtuw5RJyC8ixM2EHSWHfS0k=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBnUyEsfmF8oUOOtv2aY5xv1sqMPfmrbi/THGTaOgca WMfSFzWJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCZ1MhLAAKCRBM4MiGSL8RylHnEA Ce5tiN5nG5NiTo0pyoXJ1z06kFqbkL8qEPM0t3q+qoL7kNDG9pmQ+uBrAXK2GVn9VDIQkZl1Wo6SHM 2HHNDWrODiUqWy8ECLU4QxYpGz6dYEW5ps+gWBbUKl5NnySW8IpUngwGMnFakkDJ2fiZ3stL4E6gNd rAUP9WOYUM29bK0AQr0xyIHgq1pvIplwHl+W5rgY0j8oIZdXpzyxTOMaCNBdZm1I03xuTPf2PfOjE5 ZK3M1tAxZplYFq5aIOvOpPDq/sQF4IBIC/zYoEMxYTBPWjzMSUh3FrNex+at4sXBbOLFLn6EP1m0Fk ZgL6UeVODDGp2/qvEHFrnmQjjgHhlE0OpctFa4Bl8nMEwZLEX+bOm50XI7iAitSoV68co81YPOARiP BZsz0i6AyC+yJEG2brsj0KLNr1kvSGK6ks9ZuEX19sTlEBF0zfSuRJjs+z991Za8jo2y42FX9DGz+w 0AdzgXxxq0e+dUaJ2Y+E6YgDM9bzX3T7jrSnvXaXLJ59OT4lGWy2P5HPGmeSl2OBy0Hzb42uG0BkGZ Flw3DktFf7ibWx9sML44eDkKXcxGyNa8ydPiWLBPFFvveWQf8c16VwgMmTdW41Esq4PhytQi9XH6UZ GhBM7pROA0yAc/LOvELprDaCinLjCaUxs65xaHhLagBicgDJBakf34M/r+sw== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA X-Patchwork-Delegate: bpf@iogearbox.net Commit cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL") began marking raw_tp arguments as PTR_MAYBE_NULL, which caused the values of these pointers in the branch where they are seen to be NULL to be marked as scalar zero. In comparison, raw_tp using programs which did the NULL check never explored the NULL branch, hence successor instructions following the NULL check always saw a non-NULL raw_tp pointer and performed memory accesses on it. To preserve compatibility, we need to leave a NULL raw_tp arg as is, such that it can be allowed to be dereferenced. Otherwise, the verifer will notice the dereference of a zero scalar in the path following the NULL branch and reject existing programs. This can allow programs that do bogus things with a NULL pointer to be able to access memory (with PROBE_MEM) correctly, for instance a program only having: if (!skb) __builtin_memcpy(dst, &skb->mark, sizeof(skb->mark)); However, such programs would also not fail on earlier kernels, since the verifier would simply never explore this branch. Now, it will permit behavior where such memory is accessed and explore the branch. The correct way to fix this is to simply introduce the right annotations per-tracepoint, and remove the masking/unmasking hack, until then the raw_tp stop-gap allows programs to continue passing without deleting code that at runtime can cause safety violations. An implication of this fix, which follows from the way the raw_tp fixes were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are engulfed by these checks, and PROBE_MEM will apply to all of them, incl. those coming from helpers with KF_ACQUIRE returning maybe null trusted pointers. This NULL tagging after this commit will be sticky. Compared to a solution which only specially tagged raw_tp args with a different special maybe null tag (like PTR_SOFT_NULL), it's a consequence of overloading PTR_MAYBE_NULL with this meaning. Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL") Reported-by: Manu Bretelle Signed-off-by: Kumar Kartikeya Dwivedi --- kernel/bpf/verifier.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 82f40d63ad7b..556fb609d4a4 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env, return; if (is_null) { + /* We never mark a raw_tp trusted pointer as scalar, to + * preserve backwards compatibility, instead just leave + * it as is. + */ + if (mask_raw_tp_reg_cond(env, reg)) + return; reg->type = SCALAR_VALUE; /* We don't need id and ref_obj_id from this point * onwards anymore, thus we should better reset it, From patchwork Fri Dec 6 16:10:53 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13897359 X-Patchwork-Delegate: bpf@iogearbox.net Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 17CA520DD4C for ; Fri, 6 Dec 2024 16:11:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.65 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733501463; cv=none; b=hS5BbU6xCgaGoTa55TeXp/Iq44h0ivwKNXWm/AUigRJNsmo9hITkmorlR5FaxiZ6P/Raww4+BqPQ4AU58CGJBaGoedfYbXwua7qOpIhlf4yhNa/OAk1dWe6vnbo4jnjKZDF+yKwKBizr6R4prsTRuhtQyII6clxj56PTX/ZCtJs= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733501463; c=relaxed/simple; bh=Dmxgr9Zprr6o8ots4z4nEH9kLIYlwLIvMSD6KoW9yd4=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=q4oqQwJiUmn4sXBEPCqVQc9brYXD/oHHJf9LzvT/vGrrwVFGs47Vo34jL/y+4qdm2AzQ6+9tgxgJJ8qYiQxbQaT2NC1ItDQEC0PvHFjF3xKxnylYn9Sj9gR7KqeDdwqRBAAQC0QU5hfZ0FbKQUAJ4DWoQL/0maGRsokLwbSDLSA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=neQ1RRQ4; arc=none smtp.client-ip=209.85.128.65 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="neQ1RRQ4" Received: by mail-wm1-f65.google.com with SMTP id 5b1f17b1804b1-434a766b475so22061575e9.1 for ; Fri, 06 Dec 2024 08:11:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1733501460; x=1734106260; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=8HYDjW5kcuCcfipQTXjI09EcsauY2j6npMJPJ/1WDmg=; b=neQ1RRQ4WD1nYgIvy7fBCAsgv6vPtWVAz6vKvlf/qY9rfx880+HASKP60RvFmLAGxh j44jQIbOcK0ClazqjWGra/J1V6ttlYqi0n0pCqIYBgmOgITMNDJFAC7srcHon6s2y+Ji k0PeLNg58s9dW0txsxoJRQCtXxhLw8AQQ72oTQ1qdphSg8iRM72HsfnKtcnLFYLVWvqr 0cHhxTaC35wyzQn1tMos0/BdYG973XG2KXwbq9//snDRzQ+MuLXJkKLMhmeS+HtfOCci l97hO13x3q7wCNPa+BjzT1yUo8x1p97UM0bjZw7xBpyr3VUYfcl7Tj4xBUvi55epHXVA ZEGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733501460; x=1734106260; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=8HYDjW5kcuCcfipQTXjI09EcsauY2j6npMJPJ/1WDmg=; b=WLxNC/8Buo0lp5jEa4ZBwlprqW+evYuejIWbG2d5JAdH3JavnecDK+q6sbL0RQ37DF xwEx7iEEM4RjaDk4VnKmLfwpsHkC1tapx5CggYQl1eWlUcu3QBcet/GpPy9CHcK0XRVD nL1yUJTw6pHQfd4mDcxwUeJFEQK16bMebtin6OMb6+RG85M00DAqVM8eC3y4PWIgsGhf 2lDNO00OJ1/eL0FqaPuOuXt3G5onCHDjeugDy7IwmZqcKFQx+u20twD4iPQFKWOmRTV7 qFU+CVU699g8rjM+tpZT0O8s5c8ojodfyRIz8YXKq42jW2smFJmFlRt3FkOMDdm9KfxM DR/w== X-Gm-Message-State: AOJu0YxSAS74Q+nheN/79tJbZ6Wh0c0InD0iN8ZJbWwT/8u07AUXYhe6 UlwSJma1/ByI22POqtxBpEwHhQLjw3+ncAGXVPP7q7uH2/OeDu3194s+1XSUk8s= X-Gm-Gg: ASbGncsYng5f5pQod9+xJ/uweavVz5Y5zSNQ4rZHtFPEM4o6/VvtOa1FaOoSBOMh/FP QZ7SIqi3/yxGcoL0Lbq0PB1xa4IiJNA47Ed9E/Z8pdj4JY3XyRImTIQ81hfGp2EiXAb0YUvqWPD jiI2b/kF9HkJTfxSFtEdlb30wFzqs1Xu6dUajgLD/TdLBlHcWj06HZKqZ6UMKRHzsilukVM66tY X3fQmVZtmaZ9sWgHlWUDkzVjCnH88mdxJ11EvwHbYm7JsQPoYXOeHQCu9h4NDz8uCS5w8HHSMxN mQ== X-Google-Smtp-Source: AGHT+IF5tMtLL8GelH+5L4jipK8XQObMUHFmRAMPaAyPXzy2VRQvtqDKm+9sYGCUILx/XuEqjjDkvw== X-Received: by 2002:a05:600c:5107:b0:434:a1d3:a306 with SMTP id 5b1f17b1804b1-434ddead1c5mr31540805e9.5.1733501459923; Fri, 06 Dec 2024 08:10:59 -0800 (PST) Received: from localhost (fwdproxy-cln-017.fbsv.net. [2a03:2880:31ff:11::face:b00c]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-434d5281229sm97942455e9.26.2024.12.06.08.10.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 06 Dec 2024 08:10:59 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: kkd@meta.com, Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , Eduard Zingerman , Manu Bretelle , kernel-team@fb.com Subject: [PATCH bpf v3 3/3] selftests/bpf: Add raw_tp tests for PTR_MAYBE_NULL marking Date: Fri, 6 Dec 2024 08:10:53 -0800 Message-ID: <20241206161053.809580-4-memxor@gmail.com> X-Mailer: git-send-email 2.43.5 In-Reply-To: <20241206161053.809580-1-memxor@gmail.com> References: <20241206161053.809580-1-memxor@gmail.com> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=4652; h=from:subject; bh=Dmxgr9Zprr6o8ots4z4nEH9kLIYlwLIvMSD6KoW9yd4=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBnUyEssYcAlbxXQANgEod+HV8ecW/r5qGAJYH2HGhW e7ooEQGJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCZ1MhLAAKCRBM4MiGSL8RytQgD/ 9EWl20jfS7lnaUU56KySs6U+BfIVO7I83eqC7iCkzZDNfqlTfYkiq9khMh1BLne68XVGQ2pNLp8CN/ OXnqRfZcKSIg7B3QsFndJm/trOkd/M0vddo+RufrrHULtRPv5908akLcL6kAUqpVE09gQC+A97XQpc qhhLYW6Io9emGHHyDMxaVhtCNgseJjzwDdtdfebu/VtUhA2aQQOH1LQ+3zBiAQMQ8TfVHzA+61Wtx1 R8KQnH0FmQvQk4Kx9hVE9Sv2X7Wg/EzYvcxsD0qbTm2i7ahj0J/qpLxB0jjlmfrWBUSbOH/Mz33lLM +4n39RhRql15qYnnMygASRXulXha6b8/84eSCNFbRGtcGQIXtnqvMaqmWGIwMStGZU8XD7lOTqfm2E enBqVa8FcR2rcM8Z29HvOJTv7s5R8/oFGcoUgSRoJDsYf5Elo6DVOvC9WnnFBXcvlYf52HSuG3zb5p jxLQ3iIk4LJpvsuFWkl340YFI420MEYOFpP7t0J0pqsUnxJFD/iYRzIlfx48GQXJ1zkW0EIDHkNDBo AoYtPyKE4DxVCKLJLVQ59CWHYGKJVwRhfEXcxBYm/PQ7eWTkFOpsVyHXQ8KpI8iLg78o2Wkir8i4sd jtx5ipULTrhmaZJ9mV7a488NhRcyjv3KOGNqfoTg2irXB/QOEnea9rTWpQPg== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA X-Patchwork-Delegate: bpf@iogearbox.net Ensure that pointers with off != 0 are never unmarked as PTR_MAYBE_NULL when doing NULL checks, while pointers that have off == 0 continue to get unmarked and propagate unmarking to all other registers sharing id. Lastly, ensure that in the path where pointer is NULL, the unmarking is not performed for any registers sharing the same id. Signed-off-by: Kumar Kartikeya Dwivedi --- .../selftests/bpf/prog_tests/raw_tp_null.c | 6 ++ .../selftests/bpf/progs/raw_tp_null_fail.c | 90 +++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null_fail.c diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c index 6fa19449297e..13fcd4c31034 100644 --- a/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c +++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c @@ -3,6 +3,12 @@ #include #include "raw_tp_null.skel.h" +#include "raw_tp_null_fail.skel.h" + +void test_raw_tp_null_fail(void) +{ + RUN_TESTS(raw_tp_null_fail); +} void test_raw_tp_null(void) { diff --git a/tools/testing/selftests/bpf/progs/raw_tp_null_fail.c b/tools/testing/selftests/bpf/progs/raw_tp_null_fail.c new file mode 100644 index 000000000000..a87f25ee61ff --- /dev/null +++ b/tools/testing/selftests/bpf/progs/raw_tp_null_fail.c @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ + +#include +#include +#include "bpf_misc.h" + +/* r1 with off=0 is checked, which marks r0 with off=8 as non-null */ +SEC("tp_btf/bpf_testmod_test_raw_tp_null") +__success +__log_level(2) +__msg("3: (07) r0 += 8 ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("4: (15) if r1 == 0x0 goto pc+4 ; R1_w=trusted_ptr_sk_buff()") +__msg("5: (bf) r2 = r0 ; R0_w=trusted_ptr_sk_buff(off=8)") +/* For the path where we saw r1 as != NULL, we will see this state */ +__msg("6: (79) r2 = *(u64 *)(r1 +0) ; R1_w=trusted_ptr_sk_buff()") +/* In the NULL path, ensure registers are not marked as scalar */ +/* For the path where we saw r1 as NULL, we will see this state */ +__msg("from 4 to 9: R0=trusted_ptr_or_null_sk_buff(id=1,off=8) R1=trusted_ptr_or_null_sk_buff(id=1)") +__msg("9: (79) r2 = *(u64 *)(r1 +0) ; R1=trusted_ptr_or_null_sk_buff(id=1)") +int BPF_PROG(test_raw_tp_null_check_zero_off, struct sk_buff *skb) +{ + asm volatile ( + "r1 = *(u64 *)(r1 +0); \ + r0 = r1; \ + r2 = 0; \ + r0 += 8; \ + if r1 == 0 goto jmp; \ + r2 = r0; \ + r2 = *(u64 *)(r1 +0); \ + r0 = 0; \ + exit; \ + jmp: \ + r2 = *(u64 *)(r1 +0)" + :: + : __clobber_all + ); + return 0; +} + +/* r2 with offset is checked, which won't mark r1 with off=0 as non-NULL */ +SEC("tp_btf/bpf_testmod_test_raw_tp_null") +__success +__log_level(2) +__msg("3: (07) r2 += 8 ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("4: (15) if r2 == 0x0 goto pc+1 ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("5: (bf) r2 = r1 ; R1_w=trusted_ptr_or_null_sk_buff(id=1)") +int BPF_PROG(test_raw_tp_null_copy_check_with_off, struct sk_buff *skb) +{ + asm volatile ( + "r1 = *(u64 *)(r1 +0); \ + r2 = r1; \ + r3 = 0; \ + r2 += 8; \ + if r2 == 0 goto jmp2; \ + r2 = r1; \ + jmp2: " + :: + : __clobber_all + ); + return 0; +} + +/* Ensure state doesn't change for r0 and r1 when performing repeated checks.. */ +SEC("tp_btf/bpf_testmod_test_raw_tp_null") +__success +__log_level(2) +__msg("2: (07) r0 += 8 ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("3: (15) if r0 == 0x0 goto pc+3 ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("4: (15) if r0 == 0x0 goto pc+2 ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("5: (15) if r0 == 0x0 goto pc+1 ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("6: (bf) r2 = r1 ; R1=trusted_ptr_or_null_sk_buff(id=1)") +int BPF_PROG(test_raw_tp_check_with_off, struct sk_buff *skb) +{ + asm volatile ( + "r1 = *(u64 *)(r1 +0); \ + r0 = r1; \ + r0 += 8; \ + if r0 == 0 goto jmp3; \ + if r0 == 0 goto jmp3; \ + if r0 == 0 goto jmp3; \ + r2 = r1; \ + jmp3: " + :: + : __clobber_all + ); + return 0; +} + +char _license[] SEC("license") = "GPL";