0

I'm wanting to store the packets' payload to my custom buffer in eBPF & XDP hook. But I can't pass the verifier. I learned others' code found no difference.

In the code, I checked the length of payload should be less than MTU, which is 1500 in my code. The buffer size is (1<<20), much larger than MTU.

Here is my code in .kern :

#define BUFFER_SIZE (1<<20)
#define MTU 1500

struct my_buffer {
    __u32 len;
    char buf[BUFFER_SIZE + 5];
};
struct bpf_map_def SEC("maps") map_my_buffer = {
    .type = BPF_MAP_TYPE_ARRAY,
    .key_size = sizeof(unsigned int),
    .value_size = sizeof(struct my_buffer),
    .max_entries = 1,
};


SEC("WriteBuffer")
int WriteBuffer_main(struct xdp_md *ctx) {
    void *data_end = (void *)(long)ctx->data_end;
    void *data = (void *)(long)ctx->data;
    char *payload = data + sizeof(struct ethhdr) + sizeof(struct iphdr) + sizeof(struct udphdr);
    if (payload >= data_end) return XDP_PASS;

    unsigned int zero = 0;
    struct my_buffer *fd = bpf_map_lookup_elem(&map_my_buffer, &zero);
    if (!fd) return XDP_PASS; // can't find the context...

    __u32 data_len = data_end - (void *)payload;
    if (data_len > MTU) return XDP_PASS;

    for (__u32 i = 0; i < MTU && payload + i + 1 <= data_end; i++) {
        fd -> buf[i] = payload[i];
    }

    return XDP_DROP;
}

The error message is: It tells there is an error when accessing packet's payload, but I did the boundary check...

67: (bf) r2 = r5
68: (57) r2 &= 4064
69: (0f) r9 += r2
70: (bf) r5 = r2
71: (71) r1 = *(u8 *)(r4 -16)
72: (7b) *(u64 *)(r10 -16) = r1
73: (71) r1 = *(u8 *)(r4 -15)
invalid access to packet, off=120 size=1, R4(id=0,off=135,r=120)
R4 offset is outside of the packet
processed 60 insns (limit 1000000) max_states_per_insn 0 total_states 5 peak_states 5 mark_read 4

I'm really confused, hope someone can help out, Thanks!

Update: I followed the guide from pchaigno, and changed the loop to this:

for (__u32 i = 0; i <= MTU && i < data_len && payload + i + 1 <= data_end; ++i) {
    fd -> buf[i] = payload[i];
}

Then passed the verifier... I think it's unreasonable cause there are redundancy in these three conditions...

  • 2
    I don't think that boundary check on MTU is enough for the verifier. Try to do the boundary on `i` instead, inside the loop. We would need the full verifier output to confirm that's the issue. – pchaigno Jul 23 '22 at 06:27
  • Really thanks, I edited my loop and passed the verifier, but it's still confusing to me... – Zezhou Wang Jul 23 '22 at 08:30

1 Answers1

0

The issue is in those lines:

for (__u32 i = 0; i < MTU && payload + i + 1 <= data_end; i++) {
    fd -> buf[i] = payload[i];
}

This is a common problem: the compiler probably[1] computed and stored payload + i + 1 and payload[i] in two different registers.

Let's say the first is in R5 and the second in R6. When the compiler compared R5 to data_end, it understands that there's a new upper bound data_end for R5, but it's not smart enough to understand what that means for i. So when it then computes R6, it can't infer any upper bound for that new register.

Adding a bounds check on i solves it. The bounds check on payload + i + 1 may be unnecessary.

[1] - We would need the full verifier logs to confirm.

pchaigno
  • 11,313
  • 2
  • 29
  • 54