netdev - Re: [PATCH bpf-next v1 04/15] bpf: Allow storing referenced PTR_TO_BTF

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: <20220222065349.ladxy5cqfpdklk3a@ast-mbp.dhcp.thefacebook.com>
Date:   Mon, 21 Feb 2022 22:53:49 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v1 04/15] bpf: Allow storing referenced
 PTR_TO_BTF_ID in map

On Sun, Feb 20, 2022 at 07:18:02PM +0530, Kumar Kartikeya Dwivedi wrote:
>  static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regno,
>  			    int off, int bpf_size, enum bpf_access_type t,
> -			    int value_regno, bool strict_alignment_once)
> +			    int value_regno, bool strict_alignment_once,
> +			    struct bpf_reg_state *atomic_load_reg)

No new side effects please.
value_regno is not pretty already.
At least its known ugliness that we need to clean up one day.

>  static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
>  {
> +	struct bpf_reg_state atomic_load_reg;
>  	int load_reg;
>  	int err;
>  
> +	__mark_reg_unknown(env, &atomic_load_reg);
> +
>  	switch (insn->imm) {
>  	case BPF_ADD:
>  	case BPF_ADD | BPF_FETCH:
> @@ -4813,6 +4894,7 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>  		else
>  			load_reg = insn->src_reg;
>  
> +		atomic_load_reg = *reg_state(env, load_reg);
>  		/* check and record load of old value */
>  		err = check_reg_arg(env, load_reg, DST_OP);
>  		if (err)
> @@ -4825,20 +4907,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>  	}
>  
>  	/* Check whether we can read the memory, with second call for fetch
> -	 * case to simulate the register fill.
> +	 * case to simulate the register fill, which also triggers checks
> +	 * for manipulation of BTF ID pointers embedded in BPF maps.
>  	 */
>  	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> -			       BPF_SIZE(insn->code), BPF_READ, -1, true);
> +			       BPF_SIZE(insn->code), BPF_READ, -1, true, NULL);
>  	if (!err && load_reg >= 0)
>  		err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
>  				       BPF_SIZE(insn->code), BPF_READ, load_reg,
> -				       true);
> +				       true, load_reg >= 0 ? &atomic_load_reg : NULL);

Special xchg logic should be down outside of check_mem_access()
instead of hidden by layers of calls.