PROBLEM: Using BPF_PROG_TEST_RUN with data_out != NULL is unsafe

PROBLEM: Using BPF_PROG_TEST_RUN with data_out != NULL is unsafe

[Lorenz Bauer]

Hello,

I’ve encountered an issue when using BPF_PROG_TEST_RUN and capturing the output.
The kernel copies data into user space without checking the length of
the destination buffer.

In bpf_test_finish(), size is the amount of data in the XDP buffer /
skb after the program is run. This can be larger than data_size_in due
to bpf_xdp_adjust_head() and friends.
bpf_test_finish doesn’t clamp size to data_size_out, which is what I
was expecting.

What is the correct way to use this interface?

Best,
Lorenz

-- 
Lorenz Bauer  |  Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com

Re: PROBLEM: Using BPF_PROG_TEST_RUN with data_out != NULL is unsafe

[Daniel Borkmann]

On 04/04/2018 11:04 AM, Lorenz Bauer wrote:
> Hello,
> 
> I’ve encountered an issue when using BPF_PROG_TEST_RUN and capturing the output.
> The kernel copies data into user space without checking the length of
> the destination buffer.
> 
> In bpf_test_finish(), size is the amount of data in the XDP buffer /
> skb after the program is run. This can be larger than data_size_in due
> to bpf_xdp_adjust_head() and friends.
> bpf_test_finish doesn’t clamp size to data_size_out, which is what I
> was expecting.
> 
> What is the correct way to use this interface?

Yeah, so XDP has a headroom of XDP_PACKET_HEADROOM + NET_IP_ALIGN which in case
of x86 is 256 bytes (NET_IP_ALIGN being 0 there). This means that attr.test.data_out
buffer needs to be 256 bytes larger than attr.test.data_in in case the BPF prog
calls bpf_xdp_adjust_head() or bpf_xdp_adjust_meta(). In case you point data_in
and data_out to the same address, then the total buffer size therefore has to be
attr.test.data_size_in + 256 in order to not overrun anything while not being
aware of the BPF test program. The XDP_PACKET_HEADROOM is exposed to user space
in linux/bpf.h.

Re: PROBLEM: Using BPF_PROG_TEST_RUN with data_out != NULL is unsafe

[Lorenz Bauer]

Sorry for the late reply.

On 4 April 2018 at 11:01, Daniel Borkmann <daniel@iogearbox.net> wrote:

> In case you point data_in and data_out to the same address, then the total buffer size therefore has to be
> attr.test.data_size_in + 256 in order to not overrun anything while not being
> aware of the BPF test program. The XDP_PACKET_HEADROOM is exposed to user space
> in linux/bpf.h.

Would it be possible to extend the API of BPF_PROG_TEST_RUN to allow
user space to specify the
length of the output buffer? The kernel could then either clamp
output, or return an error.

The current API seems fundamentally hard to use, and unsafe. It leads
to kludges like [1] in a library
I'm maintaining.

1: https://github.com/newtools/ebpf/blob/f4398602ca2a37b99a1f29df9a7e8adcc57be680/prog.go#L200-L204

-- 
Lorenz Bauer  |  Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com