Re: XDP invalid memory access

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Vincent Li <mchun.li@gmail.com> writes:

> On Thu, 16 Jan 2020, Toke Høiland-Jørgensen wrote:
>
> Hi Toke,
>
>> 
>> You could also try the xdp-loader in xdp-tools:
>> https://github.com/xdp-project/xdp-tools
>> 
>> It's somewhat basic still, but should be able to at least load a basic
>> program - please file a bug report if it fails.
>
> I tried the xdp-tools xdp-loader, when the optlen is global variable, I 
> got:
> # xdp-loader load enp3s0 tcp_option.o
> Couldn't load BPF program: Relocation failed
>
> if I move the optlen, i variable to local variable, I got:
>
> # xdp-loader load enp3s0 tcp_option.o
> Couldn't load eBPF object: Invalid argument(-22)

OK, I tried this, and there were a couple of issues:

- The xdp-loader didn't set the BPF program type to XDP, and since your
  section name doesn't have an xdp_ prefix libbpf couldn't auto-detect
  it. I've pushed a fix for this to the xdp-tools repo so the loader
  will always set the program type to XDP now.

- There are a couple of bugs in your program:

First, when compiling with warnings turned on, I get this:

tcp_options.c:64:29: error: variable 'op' is uninitialized when used here [-Werror,-Wuninitialized]
                        if (op[i] == TCPOPT_EOL ) {
                            ^~
tcp_options.c:43:23: note: initialize the variable 'op' to silence this warning
        const __u8 *op;
                      ^
                       = 0

after fixing that (adding this line after the optlen = assignment):

                op = (const __u8 *)(tcphdr + 1);

the verifier then complains about out-of-bounds reading of the packet
data (pass -vv to xdp-loader to get the full debug output from libbpf).
You are not checking that the op pointer doesn't read out-of-bounds.

I fixed that by adding a couple of bounds checks inside the for loop.
The whole thing now looks like this:

                optlen = tcphdr->doff*4 - sizeof(*tcphdr);
                op = (const __u8 *)(tcphdr + 1);
                for (i = 0; i < optlen; ) {
                        if ((void *)op + i + 1 > data_end)
                                return 0;
                        if (op[i] == TCPOPT_EOL ) {
                                char fmt[] = "XDP: tcp source : %d tcp option eol\n";
                                bpf_trace_printk(fmt, sizeof(fmt), (int)tcphdr->source);
                                return 1;
                        }
                        if (op[i] < 2)
                                i++;
                        else
                                i += ((void *)op + 2 < data_end && op[i+1]) ? : 1;
                }


With this, I can successfully load the program using xdp-loader. Turning
the variables back into globals still doesn't work, but I think that
error message should be fairly obvious:

libbpf: invalid relo for 'op' in special section 0xfff2; forgot to initialize global var?..

-Toke