Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Fri, 27 Oct 2023 16:01:43 +0800
From:   Shung-Hsi Yu <shung-hsi.yu@suse.com>
To:     Hao Sun <sunhao.th@gmail.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        John Fastabend <john.fastabend@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>,
        Yonghong Song <yonghong.song@linux.dev>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>,
        Eduard Zingerman <eddyz87@gmail.com>, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH bpf-next 1/2] bpf: Fix check_stack_write_fixed_off() to
 correctly spill imm
Message-ID: <ZTtuZ1dMAN9_63Xk@u94a>
References: <20231026-fix-check-stack-write-v1-0-6b325ef3ce7e@gmail.com>
 <20231026-fix-check-stack-write-v1-1-6b325ef3ce7e@gmail.com>
 <ZTtjQlqRQWCWwmHx@u94a>
 <ZTtqSnKmaQ4ma98g@u94a>
 <CACkBjsZM8pYj6Y7vazw_Rp3ZYDnpTiCSO0bNYdt36MxFEvqZZw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACkBjsZM8pYj6Y7vazw_Rp3ZYDnpTiCSO0bNYdt36MxFEvqZZw@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dkpMQkNkeENlemM5Y2dWVG14Q1J2MldVK09INXZDUTFEVzBUWC9mcytlSUo5?=
 =?utf-8?B?Z0tTRDFMRHVkTlo0dzRyNnRVRDZXQVloZHppdXUxWlJIVm9hc1JZWU1pUGxv?=
 =?utf-8?B?S2tJbm5rdVljZkhzWFI5Lzh2d3g5UTF0bDY5czdPS1dRc2drTTNsK3NOaXRD?=
 =?utf-8?B?ejMvOEpXbVFKWWVEZjNpby9HUHF4UnNvbmhlS2FoM0NVV1BBY0xiRlJxZndq?=
 =?utf-8?B?ZVh1ckhEdFFZTng3ejVnTHVuUUw4c2RrVjVhSXNNYTBpVFRVdzYydENCWWNC?=
 =?utf-8?B?OFdHWngrckd0R3BVaVg0RDh3WnZrU2FVd3hMNW1pZm9hSlFPSFZLSENFQk43?=
 =?utf-8?B?N2tyUnNxR1k5N05rRDN1aFZkYWRpN0pTNzQ1bkFLckx4WUMycXA0dkN5RFhF?=
 =?utf-8?B?aDJHVzk4RmRzYWkwcGNEcjlYY0p6ZXppQTgzNHhhYUwwTm84bi9UZFdiem9a?=
 =?utf-8?B?MnVVWk4yM0ZFRjJSWTJUQms4NnBweXdURGVxZklmL09Ea1J6MWpzdE8zNkdv?=
 =?utf-8?B?a3pCK1U4OU1kN0hpdlhmTTNTQ0l6YWFRazAyQ2VFVE40Z0pFK3hPMmZUb2Nq?=
 =?utf-8?B?NlQxajczeExVQTRqbjFOL3oxV1F3a1lwU1dKVHpxblZFMkRvd2l2cXFoVkh0?=
 =?utf-8?B?UWR6cWFuZFBkNFNlN1ZtbXQxM3RkcDNMQ1V0SWtZekVEbk9Obi9vbXVMM1BG?=
 =?utf-8?B?MnczZTlCSVZaNWJ0OXF1dlRxMlA4K1hUNnUyeUIzZk4vdlBPQ0xOUHV4SzhD?=
 =?utf-8?B?NTF4OEtsVHQ5TDFoUFRXNUg4UmpiWThmcmg3SFllY205WmdtcUx1VXRVUlhS?=
 =?utf-8?B?S3hrUlNrbi9PTWZVZi8rUm51V3REbHRRdDBCcHJVOGhVb0oyNFRvSys3ZTVK?=
 =?utf-8?B?QWZ1R2tOUS9FN1Jjamh1R1BGa0t6QTFqWVNKVk13WGJDMHdNU2hIYzN4QU5h?=
 =?utf-8?B?eHRQWThoRUg1SEprQXNJWVJJUStZSzBPMnBoVVNpVlNYbTgrT0pRS2dNWjBv?=
 =?utf-8?B?WVd0S2k5d3RZVUo0ME9Lcjh0S2RyWmNuTnZIdVViZlNWTnMyQXZ3OEZUditD?=
 =?utf-8?B?L3FZK0VGVkpLOGV5YVpidVlrV3NWdDI4UjNQbWFmWUxac0h5TDJINktxVmlS?=
 =?utf-8?B?QnZHTG5SYVZ2S0VBNjhMc3V5VktNcnNlUWFRb2hrb2hOdEljVlJNdUlBajMz?=
 =?utf-8?B?ZDBuMTh2SUdwaVY2WCtRZTMxcEtGd25QMEM2VnhZRXVwV1ZOTGNDSUZMOVpU?=
 =?utf-8?B?d2lsZHZMaG9kc01XV0pZc05XMHNCRUdiUkRPZ09TY3QrY1RDTlpodVU1NnQz?=
 =?utf-8?B?YVJLa2lnRDBhVUdRNHVpblE5VlFyRXpIbzZZRnBGV2ZFT2ZzUktMdjhGQWVL?=
 =?utf-8?B?eGJTcnVpWTN1Y0VJNDlWK0hGTnJBMmE2RVp1WWJreXJmcmlkdmhHTnJPRmxZ?=
 =?utf-8?B?blFNaHN0WGUxWE5OR1d3MGhWRmd3QVVnL3hrV3RuNmJNOVV4TC95QmpxWCtl?=
 =?utf-8?B?OFI2NDZlYVJTeG9lWkthUnY4SEpnMnNJOHFzaWtFbGtFVC9OOE1vY3pMT0VL?=
 =?utf-8?B?WTdrclRMY0dNNStVdFE5WDRxUTVrVkZxSkU3Q0ttaHR2RC9uUUxWMzd3WTRE?=
 =?utf-8?B?ck1aampJL2xuWjVsNUpESjI2dmdNczhVdTNicWVwYTJjZlB5UjV3MjhYMlBv?=
 =?utf-8?B?N0MwYmtVbGpQKzd4TEpEeXBPZ1NQR1E1YnEyaW5YcnBNRnJsUDM4SUd0a0Zr?=
 =?utf-8?B?b1lrQkRVdFpma1FMenU3OHI3TStsYTh3Y2E5U0V6dUJ5anlJenQ0MEhmUGdo?=
 =?utf-8?B?anZUWWJRMFBzWmRTdlA1OW1IZUVqbjg1K3pPU0pBbVFhVEIxSDFKRHAzeUkr?=
 =?utf-8?B?ZGRoSnJFRVFkRHpYM2YwRXNWL0grbVpOekwrMDNkS1Z6a1ZEYVA1OGdJUXBT?=
 =?utf-8?B?ckNWci9tWURSTnRiUXY0VWRxbUlKL1lUaC9rQnVsVmovZllqYkhuSmlNUGtu?=
 =?utf-8?B?V25rRmxaaVpjdHdxMlo3KzBiK29DK1BFNFcvcnpKKzVsWXhuK3hGbkhGMjNO?=
 =?utf-8?B?bWFBcjBZRTRXUldGSTdzWmhpYitIeUZuRUZHYWJIdWpZUXJaQ1RXZkpMOFh5?=
 =?utf-8?Q?w2ZALvX82j7skMq53nckT5moq?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 999a8ba8-1a51-4867-d04c-08dbd6c2fa09
X-MS-Exchange-CrossTenant-AuthSource: AS8PR04MB9510.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Oct 2023 08:01:51.2863
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kcBkzEm61QGACDgK6M14H2FhGHkaBAyNiUf5Rwqv5a8FU9C75OgwdwiBGo3WsT1ZD/4mDktO7MxTVrtJXZfASA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB9127
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Fri, 27 Oct 2023 01:02:10 -0700 (PDT)

On Fri, Oct 27, 2023 at 09:51:58AM +0200, Hao Sun wrote:
> On Fri, Oct 27, 2023 at 9:44 AM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote:
> >
> > On Fri, Oct 27, 2023 at 03:14:10PM +0800, Shung-Hsi Yu wrote:
> > > On Thu, Oct 26, 2023 at 05:13:10PM +0200, Hao Sun wrote:
> > > > In check_stack_write_fixed_off(), imm value is cast to u32 before being
> > > > spilled to the stack. Therefore, the sign information is lost, and the
> > > > range information is incorrect when load from the stack again.
> > > >
> > > > For the following prog:
> > > > 0: r2 = r10
> > > > 1: *(u64*)(r2 -40) = -44
> > > > 2: r0 = *(u64*)(r2 - 40)
> > > > 3: if r0 s<= 0xa goto +2
> > > > 4: r0 = 1
> > > > 5: exit
> > > > 6: r0  = 0
> > > > 7: exit
> > > >
> > > > The verifier gives:
> > > > func#0 @0
> > > > 0: R1=ctx(off=0,imm=0) R10=fp0
> > > > 0: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
> > > > 1: (7a) *(u64 *)(r2 -40) = -44        ; R2_w=fp0 fp-40_w=4294967252
> > > > 2: (79) r0 = *(u64 *)(r2 -40)         ; R0_w=4294967252 R2_w=fp0
> > > > fp-40_w=4294967252
> > > > 3: (c5) if r0 s< 0xa goto pc+2
> > > > mark_precise: frame0: last_idx 3 first_idx 0 subseq_idx -1
> > > > mark_precise: frame0: regs=r0 stack= before 2: (79) r0 = *(u64 *)(r2 -40)
> > > > 3: R0_w=4294967252
> > > > 4: (b7) r0 = 1                        ; R0_w=1
> > > > 5: (95) exit
> > > > verification time 7971 usec
> > > > stack depth 40
> > > > processed 6 insns (limit 1000000) max_states_per_insn 0 total_states 0
> > > > peak_states 0 mark_read 0
> > > >
> > > > So remove the incorrect cast, since imm field is declared as s32, and
> > > > __mark_reg_known() takes u64, so imm would be correctly sign extended
> > > > by compiler.
> > > >
> > > > Signed-off-by: Hao Sun <sunhao.th@gmail.com>
> > >
> > > Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
> > >
> > > The acked-by applies to future version of the patchset as well.
> 
> (BPF_ALU | BPF_MOV | BPF_K) is handled correctly in the current
> code, i.e., no cast in BPF_ALU64 so that the sign is extended, and
> the cast in BPF_ALU correctly zero extend the reg.

My mistake, you're right. Thank you for the explanation.

> > Oh and since this is a fix it would be great to have the fixes tag[1] to
> > specify when the bug was introduced
> >
> > Fixes: ecdf985d7615 ("bpf: track immediate values written to stack by BPF_ST instruction")
> 
> Noted, thanks.
> 
> > Add Cc tag for stable[2] so stable kernels pick up the fix as well
> >
> > Cc: stable@vger.kernel.org
> >
> > And ideally specify that the patch should be applied to the bpf tree rather
> > than bpf-next[3] (though the BPF maintainers has the final say on which tree
> > this patch should be applied).
> >
> > I'd owe you a big thank as well since this helps with our internal process
> > at my company. So thank you in advance!
> >
> > 1: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> > 2: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-1
> > 3: https://docs.kernel.org/bpf/bpf_devel_QA.html#q-how-do-the-changes-make-their-way-into-linux
> >
> > > FWIW I think we'd also need the same treatment for the (BPF_ALU | BPF_MOV |
> > > BPF_K) case in check_alu_op().

^ This statement is incorrect as Hao has explained above.

> > > > ---
> > > >  kernel/bpf/verifier.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index 857d76694517..44af69ce1301 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -4674,7 +4674,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> > > >                insn->imm != 0 && env->bpf_capable) {
> > > >             struct bpf_reg_state fake_reg = {};
> > > >
> > > > -           __mark_reg_known(&fake_reg, (u32)insn->imm);
> > > > +           __mark_reg_known(&fake_reg, insn->imm);
> > > >             fake_reg.type = SCALAR_VALUE;
> > > >             save_register_state(state, spi, &fake_reg, size);
> > > >     } else if (reg && is_spillable_regtype(reg->type)) {
> > > >
> > > > --
> > > > 2.34.1
> > > >