netdev - Re: [PATCH bpf-next] bpf: Fix propagation of bounds from 64-bit min/max into 32-bit and var

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: <2d8df23d-175f-3eb8-3ba4-35659664336c@fb.com>
Date:   Fri, 29 Oct 2021 11:28:59 -0700
From:   Yonghong Song <yhs@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        <davem@...emloft.net>
CC:     <daniel@...earbox.net>, <andrii@...nel.org>,
        <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <kernel-team@...com>
Subject: Re: [PATCH bpf-next] bpf: Fix propagation of bounds from 64-bit
 min/max into 32-bit and var_off.



On 10/29/21 9:31 AM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@...nel.org>
> 
> Before this fix:
> 166: (b5) if r2 <= 0x1 goto pc+22
> from 166 to 189: R2=invP(id=1,umax_value=1,var_off=(0x0; 0xffffffff))
> 
> After this fix:
> 166: (b5) if r2 <= 0x1 goto pc+22
> from 166 to 189: R2=invP(id=1,umax_value=1,var_off=(0x0; 0x1))
> 
> While processing BPF_JLE the reg_set_min_max() would set true_reg->umax_value = 1
> and call __reg_combine_64_into_32(true_reg).
> 
> Without the fix it would not pass the condition:
> if (__reg64_bound_u32(reg->umin_value) && __reg64_bound_u32(reg->umax_value))
> 
> since umin_value == 0 at this point.
> Before commit 10bf4e83167c the umin was incorrectly ingored.
> The commit 10bf4e83167c fixed the correctness issue, but pessimized
> propagation of 64-bit min max into 32-bit min max and corresponding var_off.
> 
> Fixes: 10bf4e83167c ("bpf: Fix propagation of 32 bit unsigned bounds from 64 bit bounds")
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>

See an unrelated nits below.

Acked-by: Yonghong Song <yhs@...com>

> ---
>   kernel/bpf/verifier.c                               | 2 +-
>   tools/testing/selftests/bpf/verifier/array_access.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3c8aa7df1773..29671ed49ee8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1425,7 +1425,7 @@ static bool __reg64_bound_s32(s64 a)

We have
static bool __reg64_bound_s32(s64 a)
{
         return a > S32_MIN && a < S32_MAX;
}

Should we change to
	return a >= S32_MIN && a <= S32_MAX
?

>   
>   static bool __reg64_bound_u32(u64 a)
>   {
> -	return a > U32_MIN && a < U32_MAX;
> +	return a >= U32_MIN && a <= U32_MAX;
>   }
>   
>   static void __reg_combine_64_into_32(struct bpf_reg_state *reg)
> diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c
> index 1b1c798e9248..1b138cd2b187 100644
> --- a/tools/testing/selftests/bpf/verifier/array_access.c
> +++ b/tools/testing/selftests/bpf/verifier/array_access.c
> @@ -186,7 +186,7 @@
>   	},
>   	.fixup_map_hash_48b = { 3 },
>   	.errstr_unpriv = "R0 leaks addr",
> -	.errstr = "R0 unbounded memory access",
> +	.errstr = "invalid access to map value, value_size=48 off=44 size=8",
>   	.result_unpriv = REJECT,
>   	.result = REJECT,
>   	.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
>