linux-kernel - Re: [PATCH AUTOSEL 5.4 08/10] selftests/bpf: Fix array access with signed variable test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <X989/9omnIGyDvzV@larix.localdomain>
Date:   Sun, 20 Dec 2020 13:05:19 +0100
From:   Jean-Philippe Brucker <jean-philippe@...aro.org>
To:     Sasha Levin <sashal@...nel.org>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        linux-kselftest@...r.kernel.org, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH AUTOSEL 5.4 08/10] selftests/bpf: Fix array access with
 signed variable test

Hi,

On Sat, Dec 19, 2020 at 10:34:55PM -0500, Sasha Levin wrote:
> From: Jean-Philippe Brucker <jean-philippe@...aro.org>
> 
> [ Upstream commit 77ce220c0549dcc3db8226c61c60e83fc59dfafc ]
> 
> The test fails because of a recent fix to the verifier, even though this

That fix is commit b02709587ea3 ("bpf: Fix propagation of 32-bit signed
bounds from 64-bit bounds.") upstream, which only needed backport to 5.9.
So although backporting this patch to 5.4 shouldn't break anything, I
wouldn't bother. 

Thanks,
Jean

> program is valid. In details what happens is:
> 
>     7: (61) r1 = *(u32 *)(r0 +0)
> 
> Load a 32-bit value, with signed bounds [S32_MIN, S32_MAX]. The bounds
> of the 64-bit value are [0, U32_MAX]...
> 
>     8: (65) if r1 s> 0xffffffff goto pc+1
> 
> ... therefore this is always true (the operand is sign-extended).
> 
>     10: (b4) w2 = 11
>     11: (6d) if r2 s> r1 goto pc+1
> 
> When true, the 64-bit bounds become [0, 10]. The 32-bit bounds are still
> [S32_MIN, 10].
> 
>     13: (64) w1 <<= 2
> 
> Because this is a 32-bit operation, the verifier propagates the new
> 32-bit bounds to the 64-bit ones, and the knowledge gained from insn 11
> is lost.
> 
>     14: (0f) r0 += r1
>     15: (7a) *(u64 *)(r0 +0) = 4
> 
> Then the verifier considers r0 unbounded here, rejecting the test. To
> make the test work, change insn 8 to check the sign of the 32-bit value.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@...aro.org>
> Acked-by: John Fastabend <john.fastabend@...il.com>
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> Signed-off-by: Sasha Levin <sashal@...nel.org>
> ---
>  tools/testing/selftests/bpf/verifier/array_access.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c
> index f3c33e128709b..a80d806ead15f 100644
> --- a/tools/testing/selftests/bpf/verifier/array_access.c
> +++ b/tools/testing/selftests/bpf/verifier/array_access.c
> @@ -68,7 +68,7 @@
>  	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
>  	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 9),
>  	BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_0, 0),
> -	BPF_JMP_IMM(BPF_JSGT, BPF_REG_1, 0xffffffff, 1),
> +	BPF_JMP32_IMM(BPF_JSGT, BPF_REG_1, 0xffffffff, 1),
>  	BPF_MOV32_IMM(BPF_REG_1, 0),
>  	BPF_MOV32_IMM(BPF_REG_2, MAX_ENTRIES),
>  	BPF_JMP_REG(BPF_JSGT, BPF_REG_2, BPF_REG_1, 1),
> -- 
> 2.27.0
>