From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 77A8FC4829D for ; Tue, 13 Feb 2024 02:58:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CD0766B0075; Mon, 12 Feb 2024 21:58:41 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C7FD96B007B; Mon, 12 Feb 2024 21:58:41 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B46D56B007D; Mon, 12 Feb 2024 21:58:41 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id A634A6B0075 for ; Mon, 12 Feb 2024 21:58:41 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 2338612042A for ; Tue, 13 Feb 2024 02:58:41 +0000 (UTC) X-FDA: 81785272842.24.D806AE7 Received: from mail-wr1-f46.google.com (mail-wr1-f46.google.com [209.85.221.46]) by imf14.hostedemail.com (Postfix) with ESMTP id 718B3100013 for ; Tue, 13 Feb 2024 02:58:39 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=XIEenX+y; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf14.hostedemail.com: domain of alexei.starovoitov@gmail.com designates 209.85.221.46 as permitted sender) smtp.mailfrom=alexei.starovoitov@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707793119; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=E5DyR8fDFLIzSIkcDfBvNVsU4P0IM/S18QO0AAjZ5vI=; b=KyjvkkFc7HEeRFKQM3jeOBhmmHhp+SWXEDOiMMS93Rj/ptG+PDfiXTddYQopjy270rwAOn MdRoKjRyZgvioYB65LK/V2qshwv34mJMiONOl/k9lpjxPZ4yKjShUUN5ZuriGQAhOOPoaw Dlo2zd4QGgLEG8USOLoQGjFo4h5dww8= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=XIEenX+y; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf14.hostedemail.com: domain of alexei.starovoitov@gmail.com designates 209.85.221.46 as permitted sender) smtp.mailfrom=alexei.starovoitov@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707793119; a=rsa-sha256; cv=none; b=OZ0+DA49h2Vnj6fVoM9YLGOoP3PE80Xr4f+F0ws6CLe39If9uFBYOflt6Bk2Y6MR665McU NDSDF6mzyRgqLx91d4LWdmbV2p6H/jni+/LlcdLShD2/76TfZ3jkOAOyJIqvz4ez2roNg+ wEvgcp9VMNeV2+7qdIYYGoPCbnCd2gg= Received: by mail-wr1-f46.google.com with SMTP id ffacd0b85a97d-33cd8af482dso58601f8f.1 for ; Mon, 12 Feb 2024 18:58:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1707793118; x=1708397918; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=E5DyR8fDFLIzSIkcDfBvNVsU4P0IM/S18QO0AAjZ5vI=; b=XIEenX+y7jhCx5eUmuxmSBn7hZ/u5oEOO99JEwkQopUWX3Ifad57FDbkrIT1D/ZuXY xc1b8X4WhuxIpotGwhGc2zSgY8oBU0dVttTCwLhYEpwQykufLrf85bXIaiTOzY1RNwOc nj9xKdt1nkRP+RhpvxWRrB++2i3/JXrsopGqrAT7oPCj2VYFIbyI6Ad3r4Dw2Am/qF1y 2deWmvDR8C46LqvLjdOjBhTfUazIGg5n35bGQiYe247Gg85DvlHIKe6CtMaLk2x/xTuV FLV58/uJM8tdUvUa0yRSZU+6YGM9+LrKRKE9/7i2RsAYM+mY//XTXUOc5wjF5tEO7g5o oB+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707793118; x=1708397918; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=E5DyR8fDFLIzSIkcDfBvNVsU4P0IM/S18QO0AAjZ5vI=; b=Lo/Gs2kLtYxjtDOHvRxNXJRo4OQZOCt4FY5H4gWszq6yPZOP2tQ9BG+4AqKtbtgaBF VjdD1t8+K/Wv7Ej5zeLSB3VPj/zq8WPO45JyBNQsYh+qGEg6ZGWwbqKfOTTM7n+qbjKn 7DOfqjuIU504gfK5ixChuTi8OqzsFwEOeos7J2R9nOEbK2sgBrLmlCVoO3uP5tyw72jx sjvM6U0x80fqzHKTxZOO6fmjuIPN4/X/rg6DnNw9eh0iq6ttmAgWZ5PQw1zCDQcwf/+k 96+RGIVUAVwFAty6IbZbLKHOXusGALZz5St3qFkdQ4S9tHpHvc3VROWC7tghWC1OJbAW 9uUQ== X-Forwarded-Encrypted: i=1; AJvYcCXwc0EnfUKe3KWbsk5QEOaIzbxKaxBjOPU1ZaWLMFPb3IVik3O1ZfiD95r3jSrcrpKnn7Da+ZdcATFhFu1LlWF/rT0= X-Gm-Message-State: AOJu0Yx1MiyTJnD7oxgcBGxloyJQ1LPcaQ3To65dQcSaqL277aMIGlFX PUytSIUIqB+w0cCsIrc0ZWHwFlL4hhI/Qt7vzPmImoyQf+oKVSsplGR9nKlvLNvKSBk7cYiYrsA yNkYqxlpoTjKK8SceAOFokGYw3k4= X-Google-Smtp-Source: AGHT+IFbWRDdv8vEgmsMld/P0q++IFNkDvO1Qj/yaxFVFy3l5mfHNaa0qcOeNttbJrumnue4eXuOSr5EwjstzMgGO7U= X-Received: by 2002:a5d:56d2:0:b0:33b:1ae4:10bb with SMTP id m18-20020a5d56d2000000b0033b1ae410bbmr5902430wrw.56.1707793117709; Mon, 12 Feb 2024 18:58:37 -0800 (PST) MIME-Version: 1.0 References: <20240209040608.98927-1-alexei.starovoitov@gmail.com> <20240209040608.98927-10-alexei.starovoitov@gmail.com> In-Reply-To: From: Alexei Starovoitov Date: Mon, 12 Feb 2024 18:58:26 -0800 Message-ID: Subject: Re: [PATCH v2 bpf-next 09/20] bpf: Recognize cast_kern/user instructions in the verifier. To: Eduard Zingerman Cc: bpf , Daniel Borkmann , Andrii Nakryiko , Kumar Kartikeya Dwivedi , Tejun Heo , Barret Rhoden , Johannes Weiner , Lorenzo Stoakes , Andrew Morton , Uladzislau Rezki , Christoph Hellwig , linux-mm , Kernel Team Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 718B3100013 X-Stat-Signature: rh9aidxwjkzu99ifas5jk6rstwpdcf8h X-HE-Tag: 1707793119-943612 X-HE-Meta: U2FsdGVkX1+YZkY0/FXysMc3O4ZrFSWrONMK/KFbszkvLU0MCJbGzqoy7hA0YtYcK18zVoz87tG3Qjayt2GDDMmObF1SsOc+XN9SIn7Yj0kewsNuth+kLmEpa0vqFkwkZnTtkKRc7ycg/NaAveaensncw0wY/p2YxXDAvzRpA4hbnHsT3HHjkPRQkLIzD0CBxrVcH7ug+e4iNtDaYYTPvIq4eVDAT26x1u/rF7yWN40+mtw1h1jI+JG9d/td7k3uCPVDC4r3V0w64j/wNKzdK9/WCM1Msygkyq0//3FXenKlhcJjW+6gm+eRjNIFpVjZi6yCLaPn9XHuXC5bMQBTHzuaVndvLbZUH06prolGGip3DQj/Wcw7u0ywDeOpcRspoqiV52rDS3KleANYdyWPxwh8pg5NBf6V68nXKoTSohLBRdkh8/YXvSCLEnRCbKh1O2S6rSr8/o1vdCzp+vkLraKFmmWT5PxgDxvkplQ8s0JdNb2adyvoN/TSDz88kKWps9WIR/IracNYG69+p/jxI+5p0HYbj2I1AWch0YW47Djr/Jdpe30ovMI0k+Iq6Lb80olFnrmPHAhLaK26I4S2/A0QQSQpbJFnDji2/AJZcVpQqA0Zc67aUjySIqEJU+UWcFCNd05eURgJHuO9US/x8A0fPVramTDwksiYW9xCUSIo/HHmm9p44805iRIY+WluIw5eGKndmoxmnb5OfAjEN0pPItsm8faP93Bi4k0FVWz7PgwTu6nOri4gcIh5xBrqUFa2MvORSDq8r7Py22e/2t/LVDqsiz2nh3/RU6qLlM4GIsOzVfMXCK6lolG7AMwtDPwWmo7yAsUTWbSG0TkuD/rKERoulWuUVSxCtybH9hfkF7AxiMzaKBNIUv5Qe6Z3Ic5HIFJc6fcfAxoBMoQ9l2u5sKpmw1ES2l+wT5s8pjOpSDf692TzJ/heX3+1HpJCInLKeFiiBFcv6AeLV9r 8wAXEhKX PUWSKAC/79NYtJjmFupOrX5kETT0Ys22YXpYw4MfsZTe9MukzbzBIXucXXYx52nh7WDoGogdYmP4gLVSDex1xwSQSaO3D6mMlp1pS3nx6vf+bOpY6mU6OqGENkA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000002, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Fri, Feb 9, 2024 at 5:13=E2=80=AFPM Eduard Zingerman = wrote: > > On Thu, 2024-02-08 at 20:05 -0800, Alexei Starovoitov wrote: > [...] > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 3c77a3ab1192..5eeb9bf7e324 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > [...] > > > @@ -13837,6 +13844,21 @@ static int adjust_reg_min_max_vals(struct bpf_= verifier_env *env, > > > > dst_reg =3D ®s[insn->dst_reg]; > > src_reg =3D NULL; > > + > > + if (dst_reg->type =3D=3D PTR_TO_ARENA) { > > + struct bpf_insn_aux_data *aux =3D cur_aux(env); > > + > > + if (BPF_CLASS(insn->code) =3D=3D BPF_ALU64) > > + /* > > + * 32-bit operations zero upper bits automaticall= y. > > + * 64-bit operations need to be converted to 32. > > + */ > > + aux->needs_zext =3D true; > > It should be possible to write an example, when the same insn is > visited with both PTR_TO_ARENA and some other PTR type. > Such examples should be rejected as is currently done in do_check() > for BPF_{ST,STX} using save_aux_ptr_type(). Good catch. Fixed reg_type_mismatch_ok(). Didn't craft a unit test. That will be in a follow up. > [...] > > > @@ -13954,16 +13976,17 @@ static int check_alu_op(struct bpf_verifier_e= nv *env, struct bpf_insn *insn) > > } else if (opcode =3D=3D BPF_MOV) { > > > > if (BPF_SRC(insn->code) =3D=3D BPF_X) { > > - if (insn->imm !=3D 0) { > > - verbose(env, "BPF_MOV uses reserved field= s\n"); > > - return -EINVAL; > > - } > > - > > if (BPF_CLASS(insn->code) =3D=3D BPF_ALU) { > > - if (insn->off !=3D 0 && insn->off !=3D 8 = && insn->off !=3D 16) { > > + if ((insn->off !=3D 0 && insn->off !=3D 8= && insn->off !=3D 16) || > > + insn->imm) { > > verbose(env, "BPF_MOV uses reserv= ed fields\n"); > > return -EINVAL; > > } > > + } else if (insn->off =3D=3D BPF_ARENA_CAST_KERN |= | insn->off =3D=3D BPF_ARENA_CAST_USER) { > > + if (!insn->imm) { > > + verbose(env, "cast_kern/user insn= must have non zero imm32\n"); > > + return -EINVAL; > > + } > > } else { > > if (insn->off !=3D 0 && insn->off !=3D 8 = && insn->off !=3D 16 && > > insn->off !=3D 32) { > > I think it is now necessary to check insn->imm here, > as is it allows ALU64 move with non-zero imm. great catch too. Fixed. > > @@ -13993,7 +14016,12 @@ static int check_alu_op(struct bpf_verifier_en= v *env, struct bpf_insn *insn) > > struct bpf_reg_state *dst_reg =3D regs + insn->ds= t_reg; > > > > if (BPF_CLASS(insn->code) =3D=3D BPF_ALU64) { > > - if (insn->off =3D=3D 0) { > > + if (insn->imm) { > > + /* off =3D=3D BPF_ARENA_CAST_KERN= || off =3D=3D BPF_ARENA_CAST_USER */ > > + mark_reg_unknown(env, regs, insn-= >dst_reg); > > + if (insn->off =3D=3D BPF_ARENA_CA= ST_KERN) > > + dst_reg->type =3D PTR_TO_= ARENA; > > This effectively allows casting anything to PTR_TO_ARENA. > Do we want to check that src_reg somehow originates from arena? > Might be tricky, a new type modifier bit or something like that. Yes. Casting anything is fine. I don't think we need to enforce anything. Those insns will be llvm generated. If src_reg is somehow ptr_to_ctx or something it's likely llvm bug or crazy manual type cast by the user, but if they do so let them experience debug pains. The kernel won't crash. > > + } else if (insn->off =3D=3D 0) { > > /* case: R1 =3D R2 > > * copy register state to dest re= g > > */ > > @@ -14059,6 +14087,9 @@ static int check_alu_op(struct bpf_verifier_env= *env, struct bpf_insn *insn) > > dst_reg->subreg_def =3D e= nv->insn_idx + 1; > > coerce_subreg_to_size_sx(= dst_reg, insn->off >> 3); > > } > > + } else if (src_reg->type =3D=3D PTR_TO_AR= ENA) { > > + mark_reg_unknown(env, regs, insn-= >dst_reg); > > + dst_reg->type =3D PTR_TO_ARENA; > > This describes a case wX =3D wY, where rY is PTR_TO_ARENA, > should rX be marked as SCALAR instead of PTR_TO_ARENA? That was a leftover from earlier experiments when alu64->alu32 was done early. Removed this hunk now. > [...] > > > @@ -18235,6 +18272,31 @@ static int resolve_pseudo_ldimm64(struct bpf_v= erifier_env *env) > > fdput(f); > > return -EBUSY; > > } > > + if (map->map_type =3D=3D BPF_MAP_TYPE_ARENA) { > > + if (env->prog->aux->arena) { > > Does this have to be (env->prog->aux->arena && env->prog->aux->arena !=3D= map) ? No. all maps in used_maps[] are unique. Adding "env->prog->aux->arena !=3D map" won't make any difference. It will only be confusing. > > + verbose(env, "Only one arena per = program\n"); > > + fdput(f); > > + return -EBUSY; > > + } > > [...] > > > @@ -18799,6 +18861,18 @@ static int convert_ctx_accesses(struct bpf_ver= ifier_env *env) > > insn->code =3D=3D (BPF_ST | BPF_MEM | BPF_W) |= | > > insn->code =3D=3D (BPF_ST | BPF_MEM | BPF_DW))= { > > type =3D BPF_WRITE; > > + } else if (insn->code =3D=3D (BPF_ALU64 | BPF_MOV | BPF_X= ) && insn->imm) { > > + if (insn->off =3D=3D BPF_ARENA_CAST_KERN || > > + (((struct bpf_map *)env->prog->aux->arena)->m= ap_flags & BPF_F_NO_USER_CONV)) { > > + /* convert to 32-bit mov that clears uppe= r 32-bit */ > > + insn->code =3D BPF_ALU | BPF_MOV | BPF_X; > > + /* clear off, so it's a normal 'wX =3D wY= ' from JIT pov */ > > + insn->off =3D 0; > > + } /* else insn->off =3D=3D BPF_ARENA_CAST_USER sh= ould be handled by JIT */ > > + continue; > > + } else if (env->insn_aux_data[i + delta].needs_zext) { > > + /* Convert BPF_CLASS(insn->code) =3D=3D BPF_ALU64= to 32-bit ALU */ > > + insn->code =3D BPF_ALU | BPF_OP(insn->code) | BPF= _SRC(insn->code); > > Tbh, I think this should be done in do_misc_fixups(), > mixing it with context handling in convert_ctx_accesses() > seems a bit confusing. Good point. Moved. Thanks a lot for the review!