netdev - Re: [PATCHv3 bpf-next 04/12] bpf: Add PTR_TO

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: <8d27bf39-f6e6-1166-ce4e-c987c925ea24@iogearbox.net>
Date:   Fri, 28 Sep 2018 15:38:28 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Joe Stringer <joe@...d.net.nz>
Cc:     netdev@...r.kernel.org, ast@...nel.org, john.fastabend@...il.com,
        tgraf@...g.ch, kafai@...com, nitin.hande@...il.com,
        mauricio.vasquez@...ito.it
Subject: Re: [PATCHv3 bpf-next 04/12] bpf: Add PTR_TO_SOCKET verifier type

On 09/28/2018 01:26 AM, Joe Stringer wrote:
> Teach the verifier a little bit about a new type of pointer, a
> PTR_TO_SOCKET. This pointer type is accessed from BPF through the
> 'struct bpf_sock' structure.
> 
> Signed-off-by: Joe Stringer <joe@...d.net.nz>
[...]
> +/* Return true if it's OK to have the same insn return a different type. */
> +static bool reg_type_mismatch_ok(enum bpf_reg_type type)
> +{
> +	switch (type) {
> +	case PTR_TO_CTX:
> +	case PTR_TO_SOCKET:
> +	case PTR_TO_SOCKET_OR_NULL:
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +
> +/* If an instruction was previously used with particular pointer types, then we
> + * need to be careful to avoid cases such as the below, where it may be ok
> + * for one branch accessing the pointer, but not ok for the other branch:
> + *
> + * R1 = sock_ptr
> + * goto X;
> + * ...
> + * R1 = some_other_valid_ptr;
> + * goto X;
> + * ...
> + * R2 = *(u32 *)(R1 + 0);
> + */
> +static bool reg_type_mismatch(enum bpf_reg_type src, enum bpf_reg_type prev)
> +{
> +	return src != prev && (!reg_type_mismatch_ok(src) ||
> +			       !reg_type_mismatch_ok(prev));
> +}
> +
>  static int do_check(struct bpf_verifier_env *env)
>  {
>  	struct bpf_verifier_state *state;
> @@ -4812,9 +4894,7 @@ static int do_check(struct bpf_verifier_env *env)
>  				 */
>  				*prev_src_type = src_reg_type;
>  
> -			} else if (src_reg_type != *prev_src_type &&
> -				   (src_reg_type == PTR_TO_CTX ||
> -				    *prev_src_type == PTR_TO_CTX)) {
> +			} else if (reg_type_mismatch(src_reg_type, *prev_src_type)) {
>  				/* ABuser program is trying to use the same insn
>  				 * dst_reg = *(u32*) (src_reg + off)
>  				 * with different pointer types:
> @@ -4859,9 +4939,7 @@ static int do_check(struct bpf_verifier_env *env)
>  
>  			if (*prev_dst_type == NOT_INIT) {
>  				*prev_dst_type = dst_reg_type;
> -			} else if (dst_reg_type != *prev_dst_type &&
> -				   (dst_reg_type == PTR_TO_CTX ||
> -				    *prev_dst_type == PTR_TO_CTX)) {
> +			} else if (reg_type_mismatch(dst_reg_type, *prev_dst_type)) {
>  				verbose(env, "same insn cannot be used with different pointers\n");
>  				return -EINVAL;

Can also be as follow-up later on, but it would be crucial to also have
test_verifier tests on this logic here with mixing these pointer types
from different branches (right now we only cover ctx there).

Thanks,
Daniel