Re: [RFC bpf-next 04/11] bpf: Add PTR_TO_SOCKET verifier type

[Joe Stringer <joe@wand.net.nz>]

On 14 May 2018 at 19:37, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Wed, May 09, 2018 at 02:07:02PM -0700, 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@wand.net.nz>
>> ---
>>  include/linux/bpf.h          | 19 +++++++++-
>>  include/linux/bpf_verifier.h |  2 ++
>>  kernel/bpf/verifier.c        | 86 ++++++++++++++++++++++++++++++++++++++------
>>  net/core/filter.c            | 30 +++++++++-------
>>  4 files changed, 114 insertions(+), 23 deletions(-)
>
> Ack for patches 1-3. In this one few nits:
>
>> @@ -1723,6 +1752,16 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
>>               err = check_packet_access(env, regno, off, size, false);
>>               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 && t == BPF_READ && value_regno >= 0)
>
> t == BPF_READ check is unnecessary.
>
>> @@ -5785,7 +5845,13 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
>>
>>       if (ret == 0)
>>               /* program is valid, convert *(u32*)(ctx + off) accesses */
>> -             ret = convert_ctx_accesses(env);
>> +             ret = convert_ctx_accesses(env, env->ops->convert_ctx_access,
>> +                                        PTR_TO_CTX);
>> +
>> +     if (ret == 0)
>> +             /* Convert *(u32*)(sock_ops + off) accesses */
>> +             ret = convert_ctx_accesses(env, bpf_sock_convert_ctx_access,
>> +                                        PTR_TO_SOCKET);
>
> Overall looks great.
> Only this part is missing for PTR_TO_SOCKET:
>      } else if (dst_reg_type != *prev_dst_type &&
>                 (dst_reg_type == PTR_TO_CTX ||
>                  *prev_dst_type == PTR_TO_CTX)) {
>              verbose(env, "same insn cannot be used with different pointers\n");
>              return -EINVAL;
> similar logic has to be added.
> Otherwise the following will be accepted:
>
> R1 = sock_ptr
> goto X;
> ...
> R1 = some_other_valid_ptr;
> goto X;
> ...
>
> R2 = *(u32 *)(R1 + 0);
> this will be rewritten for first branch,
> but it's wrong for second.
>

Thanks for the review, will address these comments.