netdev - Re: [PATCH 0/3] Fix unsafe BPF_PROG_TEST

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: <040aedc7-0883-86c6-3707-b54a6a9e01c2@iogearbox.net>
Date:   Tue, 20 Nov 2018 01:34:05 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Lorenz Bauer <lmb@...udflare.com>, ys114321@...il.com
Cc:     Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org,
        linux-api@...r.kernel.org
Subject: Re: [PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface

On 11/19/2018 03:30 PM, Lorenz Bauer wrote:
> On Sun, 18 Nov 2018 at 06:13, Y Song <ys114321@...il.com> wrote:
>>
>> There is a slight change of user space behavior for this patch.
>> Without this patch, the value bpf_attr.test.data_size_out is output only.
>> For example,
>>    output buffer : out_buf (user allocated size 10)
>>    data_size_out is a random value (e.g., 1),
>>
>> The actual data to copy is 5.
>>
>> In today's implementation, the kernel will copy 5 and set data_size_out is 5.
>>
>> With this patch, the kernel will copy 1 and set data_size_out is 5.
>>
>> I am not 100% sure at this time whether we CAN overload data_size_out
>> since it MAY break existing applications.
> 
> Yes, that's correct. I think that the likelihood of this is low. It would
> affect code that uses bpf_attr without zeroing it first. I had a look around,
> and I could only find code that would keep working:

Agree, it seems like this would be rather unlikely to break the old behavior
and only if some test app forgot to zero it (given data_size_out is also in
the middle and not at the end). I'd rather prefer this approach here and then
push the patch via stable than adding yet another data_size_out-like member.

I think it also makes sense to return a -ENOSPC as Yonghong suggested in order
to indicate to user space that the buffer is not sufficient. Right now this
would have no such indication to the user so it would not be possible to
distinguish whether truncation or not happened. Was thinking whether it makes
sense to indicate through a new flag member that buffer truncation happened,
but I do like -ENOSPC better.

Thanks,
Daniel