Re: [RFC][PATCH v2 2/7] bpf: Mark ALU32 operations in bpf_reg_state structure

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Roberto Sassu <roberto.sassu@huaweicloud.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <martin.lau@linux.dev>,
	Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
	Jiri Olsa <jolsa@kernel.org>,
	Florent Revest <revest@chromium.org>,
	Brendan Jackman <jackmanb@chromium.org>,
	Mykola Lysenko <mykolal@fb.com>, Paul Moore <paul@paul-moore.com>,
	James Morris <jmorris@namei.org>,
	"Serge E . Hallyn" <serge@hallyn.com>,
	Shuah Khan <shuah@kernel.org>, bpf <bpf@vger.kernel.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Roberto Sassu <roberto.sassu@huawei.com>
Subject: Re: [RFC][PATCH v2 2/7] bpf: Mark ALU32 operations in bpf_reg_state structure
Date: Sat, 10 Dec 2022 18:28:18 -0800	[thread overview]
Message-ID: <CAADnVQKhWEtqAkMnWR8Twpc6uPo_MWnAf68R-xeM=YVqxkLOyQ@mail.gmail.com> (raw)
In-Reply-To: <20221207172434.435893-3-roberto.sassu@huaweicloud.com>

On Wed, Dec 7, 2022 at 9:25 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> BPF LSM needs a reliable source of information to determine if the return
> value given by eBPF programs is acceptable or not. At the moment, choosing
> either the 64 bit or the 32 bit one does not seem to be an option
> (selftests fail).
>
> If we choose the 64 bit one, the following happens.
>
>       14:       61 10 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
>       15:       74 00 00 00 15 00 00 00 w0 >>= 21
>       16:       54 00 00 00 01 00 00 00 w0 &= 1
>       17:       04 00 00 00 ff ff ff ff w0 += -1
>
> This is the last part of test_deny_namespace. After #16, the register
> values are:
>
> smin_value = 0x0, smax_value = 0x1,
> s32_min_value = 0x0, s32_max_value = 0x1,
>
> After #17, they become:
>
> smin_value = 0x0, smax_value = 0xffffffff,
> s32_min_value = 0xffffffff, s32_max_value = 0x0
>
> where only the 32 bit values are correct.
>
> If we choose the 32 bit ones, the following happens.
>
> 0000000000000000 <check_access>:
>        0:       79 12 00 00 00 00 00 00 r2 = *(u64 *)(r1 + 0)
>        1:       79 10 08 00 00 00 00 00 r0 = *(u64 *)(r1 + 8)
>        2:       67 00 00 00 3e 00 00 00 r0 <<= 62
>        3:       c7 00 00 00 3f 00 00 00 r0 s>>= 63
>
> This is part of test_libbpf_get_fd_by_id_opts (no_alu32 version). In this
> case, 64 bit register values should be used (for the 32 bit ones, there is
> no precise information from the verifier).
>
> As the examples above suggest that which register values to use depends on
> the specific case, mark ALU32 operations in bpf_reg_state structure, so
> that BPF LSM can choose the proper ones.

I have a hard time understanding what is the problem you're
trying to solve and what is the proposed fix.

The patch is trying to remember the bitness of the last
operation, but what for?
The registers are 64-bit. There are 32-bit operations,
but they always update the upper 32-bits of the register.
reg_bounds_sync() updates 32 and 64 bit bounds regardless
whether the previous operation was on 32 or 64 bit.
It seems you're trying to hack around something that breaks
patch 3 which also looks fishy.
Please explain the problem first with a concrete example.