netdev - Re: [PATCH bpf-next 03/13] bpf: support readonly buffer in verifier

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: <20200713232545.mmocpqgqpiapcdvg@ast-mbp.dhcp.thefacebook.com>
Date:   Mon, 13 Jul 2020 16:25:45 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, kernel-team@...com,
        Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf-next 03/13] bpf: support readonly buffer in verifier

On Mon, Jul 13, 2020 at 09:17:42AM -0700, Yonghong Song wrote:
> Two new readonly buffer PTR_TO_RDONLY_BUF or
> PTR_TO_RDONLY_BUF_OR_NULL register states
> are introduced. These new register states will be used
> by later bpf map element iterator.
> 
> New register states share some similarity to
> PTR_TO_TP_BUFFER as it will calculate accessed buffer
> size during verification time. The accessed buffer
> size will be later compared to other metrics during
> later attach/link_create time.
> 
> Two differences between PTR_TO_TP_BUFFER and
> PTR_TO_RDONLY_BUF[_OR_NULL].
> PTR_TO_TP_BUFFER is for write only
> and PTR_TO_RDONLY_BUF[_OR_NULL] is for read only.
> In addition, a rdonly_buf_seq_id is also added to the
> register state since it is possible for the same program
> there could be two PTR_TO_RDONLY_BUF[_OR_NULL] ctx arguments.
> For example, for bpf later map element iterator,
> both key and value may be PTR_TO_TP_BUFFER_OR_NULL.
> 
> Similar to reg_state PTR_TO_BTF_ID_OR_NULL in bpf
> iterator programs, PTR_TO_RDONLY_BUF_OR_NULL reg_type and
> its rdonly_buf_seq_id can be set at
> prog->aux->bpf_ctx_arg_aux, and bpf verifier will
> retrieve the values during btf_ctx_access().
> Later bpf map element iterator implementation
> will show how such information will be assigned
> during target registeration time.
...
>  struct bpf_ctx_arg_aux {
>  	u32 offset;
>  	enum bpf_reg_type reg_type;
> +	u32 rdonly_buf_seq_id;
>  };
>  
> +#define BPF_MAX_RDONLY_BUF	2
> +
>  struct bpf_prog_aux {
>  	atomic64_t refcnt;
>  	u32 used_map_cnt;
> @@ -693,6 +699,7 @@ struct bpf_prog_aux {
>  	u32 attach_btf_id; /* in-kernel BTF type id to attach to */
>  	u32 ctx_arg_info_size;
>  	const struct bpf_ctx_arg_aux *ctx_arg_info;
> +	u32 max_rdonly_access[BPF_MAX_RDONLY_BUF];

I think PTR_TO_RDONLY_BUF approach is too limiting.
I think the map value should probably be writable from the beginning,
but I don't see how this RDONLY_BUF support can be naturally extended.
Also key and value can be large, so just load/store is going to be
limiting pretty quickly. People would want to use helpers to access
key/value areas. I think any existing helper that accepts ARG_PTR_TO_MEM
should be usable with data from this key/value.
PTR_TO_TP_BUFFER was a quick hack for tiny scratch area.
Here I think the verifier should be smart from the start.

The next patch populates bpf_ctx_arg_aux with hardcoded 0 and 1.
imo that's too hacky. Helper definitions shouldn't be in business
of poking into such verifier internals.