[0/3] Fix unsafe BPF_PROG_TEST_RUN interface

Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
This is because bpf_test_finish copies the output buffer to user space
without checking its size. This can lead to the kernel overwriting
data in user space after the buffer if xdp_adjust_head and friends are
in play.

Fix this by using bpf_attr.test.data_size_out as a size hint. The old
behaviour is retained if size_hint is zero.

Interestingly, do_test_single() in test_verifier.c already assumes
that this is the intended use of data_size_out, and sets it to the
output buffer size.

Lorenz Bauer (3):
  bpf: respect size hint to BPF_PROG_TEST_RUN if present
  libbpf: require size hint in bpf_prog_test_run
  selftests: add a test for bpf_prog_test_run output size

 net/bpf/test_run.c                       |  9 ++++-
 tools/lib/bpf/bpf.c                      |  4 ++-
 tools/testing/selftests/bpf/test_progs.c | 44 ++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 2 deletions(-)

On Fri, Nov 16, 2018 at 12:54 PM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
> This is because bpf_test_finish copies the output buffer to user space
> without checking its size. This can lead to the kernel overwriting
> data in user space after the buffer if xdp_adjust_head and friends are
> in play.
>
> Fix this by using bpf_attr.test.data_size_out as a size hint. The old
> behaviour is retained if size_hint is zero.

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.

Alternativley, we can append another field to bpf_attr.test
  __u32 data_out_size;
this will provide the data_out buffer size.
Inside kernel, if the user input attr is smaller than kernel and does not
have data_out_size, the current behavior should be used. Otherwise,
data_out_size is data_out buffer size.

Daniel and Alexei, which option do you think is reasonable?

>
> Interestingly, do_test_single() in test_verifier.c already assumes
> that this is the intended use of data_size_out, and sets it to the
> output buffer size.
>
> Lorenz Bauer (3):
>   bpf: respect size hint to BPF_PROG_TEST_RUN if present
>   libbpf: require size hint in bpf_prog_test_run
>   selftests: add a test for bpf_prog_test_run output size
>
>  net/bpf/test_run.c                       |  9 ++++-
>  tools/lib/bpf/bpf.c                      |  4 ++-
>  tools/testing/selftests/bpf/test_progs.c | 44 ++++++++++++++++++++++++
>  3 files changed, 55 insertions(+), 2 deletions(-)
>
> --
> 2.17.1
>

On Sun, 18 Nov 2018 at 06:13, Y Song <ys114321@gmail.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:

* kernel libbpf and descendants (e.g katran)
* github.com/iovisor/gobpf
* github.com/newtools/ebpf

That doesn't really guarantee anything of course.

On 11/19/2018 03:30 PM, Lorenz Bauer wrote:
> On Sun, 18 Nov 2018 at 06:13, Y Song <ys114321@gmail.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