From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mta0.migadu.com (out-57.mta0.migadu.com [91.218.175.57]) (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 0816F3EFD26 for ; Fri, 11 Sep 2026 06:07:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.57 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789106881; cv=none; b=lYaSCZ6VkyCMfuxxaNKdTcPTJ8DK8VLlSCN07ncbclAreU531hAPSdf6sGd0eSJ2tT8ZvHteTMppVJsM1Q2HsOIbkpnRb5kqGuzFd8ypLUs3HhBA6PsaHbHgOfcrJ1otmk9ssJzRBBt4mgxLKTsF23XfFthOWHePE5JMGvthuQI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789106881; c=relaxed/simple; bh=h8GPIkniRK73ShW81tfgfiofelIBlcP5lDIZ39GiHJ8=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=KkYRNd+LhKYDBYXnJy6WFjvLW+jKH1w0ov9kfbBCUyj6r8ASXNdVtlPipuIDjpDUwdy1D+bQcVWr4FSHSn/hIQ5BGfYtvxi4f0etG6dPXVOkbaAD0gUx1E/I8O38Gm5MDuFwtyAXHoyDC/KRfY9tit5l9chs29FX/x17FcsW2DA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=ev4lC9LE; arc=none smtp.client-ip=91.218.175.57 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="ev4lC9LE" X-Envelope-To: bpf@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=h8GPIkniRK73ShW81tfgfiofelIBlcP5lDIZ39GiHJ8=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1789106876; v=1; x=1789711676; b=ev4lC9LErNn4ru5bOwEWWqRpmAamJAiJd8Ny+zgmMyccPEJLnTMgRJCdkRXwUDQN/E6cM2RB 2Cl//2KLNxQsveW8jzH8RpwhVR1fGArYsCB88zpIcmJ0SmZizS7btmfWCsy3+u+insJ8MmVVnY2 C5QRJpjM7kCBfp1FCEhycLe0= X-Envelope-To: bpf@vger.kernel.org Received: by smtp.migadu.com with ESMTPS id af4da63a47fc018e; Fri, 11 Sep 2026 06:07:56 +0000 X-Mizu-Trace-ID: af4da63a47fc018e X-Migadu-Flow: FLOW_OUT Message-ID: <3f3e5658-4393-4530-8b3b-987fe20ddc95@linux.dev> Date: Fri, 11 Sep 2026 11:37:52 +0530 Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH bpf-next v2 09/13] bpf: track low-32 scalar equality across narrowing stack fills To: sashiko-reviews@lists.linux.dev Cc: bpf@vger.kernel.org References: <20260910164635.459558-1-vineet.gupta@linux.dev> <20260910164635.459558-10-vineet.gupta@linux.dev> <20260910170419.4C8601F000FF@smtp.kernel.org> From: Vineet Gupta Content-Language: en-US In-Reply-To: <20260910170419.4C8601F000FF@smtp.kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 9/10/26 10:34 PM, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > - [Critical] Sign-extending loads incorrectly preserve conflicting SUBREG_ZEXT metadata. > -- > > commit f939550a777ae3d2a7392dec744bffdc9fabcdda > Author: Vineet Gupta > > bpf: track low-32 scalar equality across narrowing stack fills > > This commit records a low-32 link for stack fills narrower than the wider > source, allowing the verifier to deduce relations between 32-bit narrows > and their wider origins. It also adjusts check_mem_access() to not clear > the id of a ->subreg register on a sign-extending load. > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 308ff53232f06..89be1240c99ae 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c > [ ... ] > >> @@ -3968,18 +3969,34 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, >> >> if (size <= spill_size && >> bpf_stack_narrow_access_ok(off, size, spill_size)) { >> - if (env->bpf_capable && size == 4 && spill_size == 4 && >> - get_reg_width(reg) <= 32) >> + bool narrowing = get_reg_width(reg) > size * BITS_PER_BYTE; >> + /* >> + * A narrowing fill keeps only the slot's low 32 bits, >> + * so record a low-32 link rather than dropping the >> + * relation, as a 32-bit mov from a wide source does. >> + * Which kind depends on how the load fills the high >> + * half, hence is_ldsx. >> + */ >> + bool subreg_link = narrowing && size == 4; >> + >> + if (env->bpf_capable && size == 4 && >> + (subreg_link || (spill_size == 4 && !narrowing))) >> /* Ensure stack slot has an ID to build a relation >> * with the destination register on fill. >> */ >> assign_scalar_id_before_mov(env, reg); >> state->regs[dst_regno] = *reg; >> >> - /* Break the relation on a narrowing fill. >> - * coerce_reg_to_size will adjust the boundaries. >> - */ >> - if (get_reg_width(reg) > size * BITS_PER_BYTE) >> + if (subreg_link && reg->id) >> + state->regs[dst_regno].subreg = >> + is_ldsx ? SUBREG_SEXT : SUBREG_ZEXT; >> + else if (narrowing) >> + /* >> + * Nothing to relate: either the slot has >> + * no id to share, or the fill is narrower >> + * than the 32 bits a link can describe. >> + * coerce_reg_to_size adjusts the bounds. >> + */ >> clear_scalar_id(&state->regs[dst_regno]); > [Severity: Critical] > When narrowing is false, does this preserve a stale SUBREG_ZEXT property for > a sign-extending load? > > If a register with SUBREG_ZEXT is spilled to the stack and then read back > via a 4-byte sign-extending load, narrowing evaluates to false. This allows > the destination register to inherit the SUBREG_ZEXT property directly from > the stack slot, even though it is being sign-extended. > > [ ... ] Indeed if narrowing is false, wrong metadata can potentially be introduced, a comment to that effect is added here. /*  * A fill that is not narrowing copies the slot's  * kind verbatim, which a sign-extending load can  * contradict. check_mem_access() runs after this  * and drops any kind but SUBREG_SEXT once it knows  * the extension was not a no-op.  */ See the second site below for actual fix... >> @@ -6736,13 +6753,15 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b >> * Sign-extension can change the register value relative >> * to a scalar it is linked with by id (e.g. a zero- >> * extending fill of the same spilled stack slot), thus >> - * drop the shared id in that case. >> + * drop the shared id in that case. A ->subreg link is >> + * the exception: it already records that only the low >> + * 32 bits are shared, and how the high half follows. >> */ >> bool no_sext = reg_umax(®s[value_regno]) < >> (1ULL << (size * BITS_PER_BYTE - 1)); >> >> coerce_reg_to_size_sx(®s[value_regno], size); >> - if (!no_sext) >> + if (!no_sext && !regs[value_regno].subreg) >> clear_scalar_id(®s[value_regno]); > [Severity: Critical] > Does this check wrongly preserve the ID for SUBREG_ZEXT registers that > have just been sign-extended? > > Because SUBREG_ZEXT is non-zero, !regs[value_regno].subreg evaluates to > false, skipping the clear_scalar_id() call for a register that might hold > a sign-extended negative value. > > Later, if a conditional branch triggers sync_linked_regs(), it will see > SUBREG_ZEXT and invoke reconstruct_zext32(). This would forcefully set the > bounds to a zero-extended positive value. > > At runtime, the discrepancy between the verifier's zero-extended positive > bounds and the actual sign-extended negative value could bypass bounds checks, > potentially leading to out-of-bounds memory reads or writes. > > Should this explicitly check for SUBREG_SEXT rather than just allowing any > non-zero ->subreg value? Exactly that. And the first path leads here so the comment above suffices. Thx, -Vineet