netdev - Re: [PATCH v6 bpf-next 01/17] bpf: verifier: offer more accurate helper function arg and return type

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]

Date:   Mon, 6 May 2019 15:57:23 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jiong Wang <jiong.wang@...ronome.com>, alexei.starovoitov@...il.com
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH v6 bpf-next 01/17] bpf: verifier: offer more accurate
 helper function arg and return type

On 05/03/2019 12:42 PM, Jiong Wang wrote:
> BPF helper call transfers execution from eBPF insns to native functions
> while verifier insn walker only walks eBPF insns. So, verifier can only
> knows argument and return value types from explicit helper function
> prototype descriptions.
> 
> For 32-bit optimization, it is important to know whether argument (register
> use from eBPF insn) and return value (register define from external
> function) is 32-bit or 64-bit, so corresponding registers could be
> zero-extended correctly.
> 
> For arguments, they are register uses, we conservatively treat all of them
> as 64-bit at default, while the following new bpf_arg_type are added so we
> could start to mark those frequently used helper functions with more
> accurate argument type.
> 
>   ARG_CONST_SIZE32
>   ARG_CONST_SIZE32_OR_ZERO

For the above two, I was wondering is there a case where the passed size is
not used as 32 bit aka couldn't we generally assume 32 bit here w/o adding
these two extra arg types? For ARG_ANYTHING32 and RET_INTEGER64 definitely
makes sense (btw, opt-in value like RET_INTEGER32 might have been easier for
reviewing converted helpers).

>   ARG_ANYTHING32
> 
> A few helper functions shown up frequently inside Cilium bpf program are
> updated using these new types.
> 
> For return values, they are register defs, we need to know accurate width
> for correct zero extensions. Given most of the helper functions returning
> integers return 32-bit value, a new RET_INTEGER64 is added to make those
> functions return 64-bit value. All related helper functions are updated.
> 
> Signed-off-by: Jiong Wang <jiong.wang@...ronome.com>
[...]

> @@ -2003,9 +2003,9 @@ static const struct bpf_func_proto bpf_csum_diff_proto = {
>  	.pkt_access	= true,
>  	.ret_type	= RET_INTEGER,
>  	.arg1_type	= ARG_PTR_TO_MEM_OR_NULL,
> -	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,
> +	.arg2_type	= ARG_CONST_SIZE32_OR_ZERO,
>  	.arg3_type	= ARG_PTR_TO_MEM_OR_NULL,
> -	.arg4_type	= ARG_CONST_SIZE_OR_ZERO,
> +	.arg4_type	= ARG_CONST_SIZE32_OR_ZERO,
>  	.arg5_type	= ARG_ANYTHING,
>  };

I noticed that the above and also bpf_csum_update() would need to be converted
to RET_INTEGER64 as they would break otherwise: it's returning error but also
u32 csum value, so use for error checking would be s64 ret = bpf_csum_xyz(...).

Thanks,
Daniel