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: <939a8eac-deaa-771c-c7a6-94c2271dea40@iogearbox.net>
Date:   Fri, 28 Sep 2018 15:29:33 +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

Hi Joe,

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>
[...]
>  	}
> @@ -1726,6 +1755,14 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>  		err = check_flow_keys_access(env, off, size);
>  		if (!err && t == BPF_READ && value_regno >= 0)
>  			mark_reg_unknown(env, regs, value_regno);
> +	} else if (reg->type == PTR_TO_SOCKET) {
> +		if (t == BPF_WRITE) {
> +			verbose(env, "cannot write into socket\n");
> +			return -EACCES;
> +		}
> +		err = check_sock_access(env, regno, off, size, t);
> +		if (!err && value_regno >= 0)
> +			mark_reg_unknown(env, regs, value_regno);

Not an issue today, but this is quite fragile and very easy to miss, if we
allow to enable writes into ptr_to_socket in future e.g. mark or others,
then lifting above will not be enough. E.g. see check_xadd() and friends,
this rejects writes to ctx via f37a8cb84cce ("bpf: reject stores into ctx
via st and xadd") as otherwise this would bypass the context rewriter. So
I think we should add PTR_TO_SOCKET to is_ctx_reg() as well to have a full
guarantee this won't happen.

>  	} else {
>  		verbose(env, "R%d invalid mem access '%s'\n", regno,
>  			reg_type_str[reg->type]);

Thanks,
Daniel