netdev - Re: [PATCH bpf 7/7] bpf: make direct packet write unclone more robust

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a851d791-8477-75aa-f0ca-0400c841f987@iogearbox.net>
Date:   Thu, 25 Oct 2018 00:08:05 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Song Liu <liu.song.a23@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf 7/7] bpf: make direct packet write unclone more robust

On 10/24/2018 11:42 PM, Song Liu wrote:
> On Wed, Oct 24, 2018 at 1:06 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>>
>> Given this seems to be quite fragile and can easily slip through the
>> cracks, lets make direct packet write more robust by requiring that
>> future program types which allow for such write must provide a prologue
>> callback. In case of XDP and sk_msg it's noop, thus add a generic noop
>> handler there. The latter starts out with NULL data/data_end unconditionally
>> when sg pages are shared.
>>
>> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
>> Acked-by: Alexei Starovoitov <ast@...nel.org>
>> ---
>>  kernel/bpf/verifier.c |  6 +++++-
>>  net/core/filter.c     | 11 +++++++++++
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 5fc9a65..171a2c8 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -5709,7 +5709,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>>         bool is_narrower_load;
>>         u32 target_size;
>>
>> -       if (ops->gen_prologue) {
>> +       if (ops->gen_prologue || env->seen_direct_write) {
>> +               if (!ops->gen_prologue) {
>> +                       verbose(env, "bpf verifier is misconfigured\n");
>> +                       return -EINVAL;
>> +               }
> 
> nit: how about this?
> 
> diff --git i/kernel/bpf/verifier.c w/kernel/bpf/verifier.c
> index 6fbe7a8afed7..d35078024e35 100644
> --- i/kernel/bpf/verifier.c
> +++ w/kernel/bpf/verifier.c
> @@ -5286,6 +5286,11 @@ static int convert_ctx_accesses(struct
> bpf_verifier_env *env)
>         bool is_narrower_load;
>         u32 target_size;
> 
> +       if (!ops->gen_prologue && env->seen_direct_write) {
> +               verbose(env, "bpf verifier is misconfigured\n");
> +               return -EINVAL;
> +       }
> +
>         if (ops->gen_prologue) {
>                 cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
>                                         env->prog);
> 

Hm, probably matter of different style preference, personally I'd prefer
the one as is though.

Thanks,
Daniel