iovisor / iovisor/bcc

String loaded using BPF_probe_read being overridden by constant string in some cases

Open
#2,352 7 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
22.7k
Forks
4.1k
Avg merge
10d 4h
Merged PRs (30d)
3

Description

LLVM: 6, 7 and 8
BCC: 0.9
Kernel: 4.15.0-47-generic

In some cases, strings loaded with BPF_probe_read are being overridden with constant strings. This seems like an LLVM optimization issue.

The code below works as expeced:

```python
#!/usr/bin/env python

from bcc import BPF
from time import sleep

text = """
#include
#include

#define NODENAME_LEN __NEW_UTS_LEN

typedef struct Foo {
char loaded[NODENAME_LEN];
char constant[10];
} Foo_t;

BPF_HASH(foos, Foo_t);

int bug_example(struct pt_regs *ctx) {
char constant[] = "mystr";
struct task_struct *task = (struct task_struct *)bpf_get_current_task();

char loaded[NODENAME_LEN] = "";
__builtin_memcpy(loaded, task->nsproxy->uts_ns->name.nodename, NODENAME_LEN);

Foo_t foo;
__builtin_memcpy(&foo.loaded, loaded, sizeof(foo.loaded));
__builtin_memcpy(&foo.constant, constant, sizeof(foo.constant));

u64 ts = 1;
foos.update(&foo, &ts);

return 0;
}
"""

b = BPF(text=text, debug=0)
b.attach_kprobe(event="do_nanosleep", fn_name="bug_example")
sleep(2)
counts = b.get_table("foos")
for k, v in counts.items():
print("%10d \"%s\" \"%s\"" % (v.value, k.loaded, k.constant))
```

Output:

```
# python bug.py
1 "laul-mmarchini.corp.netflix.com" "mystr"
1 "laul-mmarchini" "mystr"
```

But if we swap `loaded` and `constant` in the struct `Foo`, the `loaded` string will be overriden with the `constant` string, leading to a erroneous result.

```diff
@@ -12,8 +12,8 @@


typedef struct Foo {
- char loaded[NODENAME_LEN];
char constant[10];
+ char loaded[NODENAME_LEN];
} Foo_t;

BPF_HASH(foos, Foo_t);
```

Output:

```
1 "mystr" "mystr"
1 "mystr" "mystr"
1 "mystr" "mystr"
```

This is the correct BPF program:

```
0: (85) call bpf_get_current_task#35
1: (b7) r6 = 0
2: (73) *(u8 *)(r10 -32) = r6
3: (7b) *(u64 *)(r10 -40) = r6
4: (7b) *(u64 *)(r10 -48) = r6
5: (7b) *(u64 *)(r10 -56) = r6
6: (7b) *(u64 *)(r10 -64) = r6
7: (7b) *(u64 *)(r10 -72) = r6
8: (7b) *(u64 *)(r10 -80) = r6
9: (7b) *(u64 *)(r10 -88) = r6
10: (7b) *(u64 *)(r10 -96) = r6
11: (7b) *(u64 *)(r10 -8) = r6
12: (7b) *(u64 *)(r10 -16) = r6
13: (07) r0 += 2712
14: (bf) r1 = r10
15: (07) r1 += -16
16: (b7) r2 = 8
17: (bf) r3 = r0
18: (85) call bpf_probe_read#4
19: (79) r3 = *(u64 *)(r10 -16)
20: (07) r3 += 8
21: (bf) r1 = r10
22: (07) r1 += -8
23: (b7) r2 = 8
24: (85) call bpf_probe_read#4
25: (79) r3 = *(u64 *)(r10 -8)
26: (07) r3 += 69
27: (bf) r1 = r10
28: (07) r1 += -96
29: (b7) r2 = 65
30: (85) call bpf_probe_read#4
31: (18) r1 = 0x727473796d
33: (7b) *(u64 *)(r10 -32) = r1
34: (79) r1 = *(u64 *)(r10 -40)
35: (7b) *(u64 *)(r10 -40) = r1
36: (79) r1 = *(u64 *)(r10 -48)
37: (7b) *(u64 *)(r10 -48) = r1
38: (79) r1 = *(u64 *)(r10 -56)
39: (7b) *(u64 *)(r10 -56) = r1
40: (79) r1 = *(u64 *)(r10 -64)
41: (7b) *(u64 *)(r10 -64) = r1
42: (79) r1 = *(u64 *)(r10 -72)
43: (7b) *(u64 *)(r10 -72) = r1
44: (79) r1 = *(u64 *)(r10 -80)
45: (7b) *(u64 *)(r10 -80) = r1
46: (79) r1 = *(u64 *)(r10 -88)
47: (7b) *(u64 *)(r10 -88) = r1
48: (79) r1 = *(u64 *)(r10 -96)
49: (7b) *(u64 *)(r10 -96) = r1
50: (6b) *(u16 *)(r10 -24) = r6
51: (b7) r1 = 1
52: (7b) *(u64 *)(r10 -8) = r1
53: (18) r1 = 0xffff97f3091f1600
55: (bf) r2 = r10
56: (07) r2 += -96
57: (bf) r3 = r10
58: (07) r3 += -8
59: (b7) r4 = 0
60: (85) call bpf_map_update_elem#2
61: (b7) r0 = 0
62: (95) exit
processed 61 insns, stack depth 96
```

And this is the faulty BPF program:

```
0: (85) call bpf_get_current_task#35
1: (b7) r6 = 0
2: (73) *(u8 *)(r10 -32) = r6
3: (7b) *(u64 *)(r10 -40) = r6
4: (7b) *(u64 *)(r10 -48) = r6
5: (7b) *(u64 *)(r10 -56) = r6
6: (7b) *(u64 *)(r10 -64) = r6
7: (7b) *(u64 *)(r10 -72) = r6
8: (7b) *(u64 *)(r10 -80) = r6
9: (7b) *(u64 *)(r10 -88) = r6
10: (7b) *(u64 *)(r10 -96) = r6
11: (7b) *(u64 *)(r10 -8) = r6
12: (7b) *(u64 *)(r10 -16) = r6
13: (07) r0 += 2712
14: (bf) r1 = r10
15: (07) r1 += -16
16: (b7) r2 = 8
17: (bf) r3 = r0
18: (85) call bpf_probe_read#4
19: (79) r3 = *(u64 *)(r10 -16)
20: (07) r3 += 8
21: (bf) r1 = r10
22: (07) r1 += -8
23: (b7) r2 = 8
24: (85) call bpf_probe_read#4
25: (79) r3 = *(u64 *)(r10 -8)
26: (07) r3 += 69
27: (bf) r1 = r10
28: (07) r1 += -96
29: (b7) r2 = 65
30: (85) call bpf_probe_read#4
31: (18) r1 = 0x727473796d
33: (7b) *(u64 *)(r10 -96) = r1
34: (79) r1 = *(u64 *)(r10 -40)
35: (bf) r2 = r1
36: (77) r2 >>= 48
37: (6b) *(u16 *)(r10 -24) = r2
38: (bf) r2 = r1
39: (77) r2 >>= 32
40: (6b) *(u16 *)(r10 -26) = r2
41: (6b) *(u16 *)(r10 -30) = r1
42: (77) r1 >>= 16
43: (6b) *(u16 *)(r10 -28) = r1
44: (79) r1 = *(u64 *)(r10 -48)
45: (bf) r2 = r1
46: (77) r2 >>= 48
47: (6b) *(u16 *)(r10 -32) = r2
48: (bf) r2 = r1
49: (77) r2 >>= 32
50: (6b) *(u16 *)(r10 -34) = r2
51: (6b) *(u16 *)(r10 -38) = r1
52: (77) r1 >>= 16
53: (6b) *(u16 *)(r10 -36) = r1
54: (79) r1 = *(u64 *)(r10 -56)
55: (bf) r2 = r1
56: (77) r2 >>= 48
57: (6b) *(u16 *)(r10 -40) = r2
58: (bf) r2 = r1
59: (77) r2 >>= 32
60: (6b) *(u16 *)(r10 -42) = r2
61: (6b) *(u16 *)(r10 -46) = r1
62: (77) r1 >>= 16
63: (6b) *(u16 *)(r10 -44) = r1
64: (79) r1 = *(u64 *)(r10 -64)
65: (bf) r2 = r1
66: (77) r2 >>= 48
67: (6b) *(u16 *)(r10 -48) = r2
68: (bf) r2 = r1
69: (77) r2 >>= 32
70: (6b) *(u16 *)(r10 -50) = r2
71: (6b) *(u16 *)(r10 -54) = r1
72: (77) r1 >>= 16
73: (6b) *(u16 *)(r10 -52) = r1
74: (79) r1 = *(u64 *)(r10 -72)
75: (bf) r2 = r1
76: (77) r2 >>= 48
77: (6b) *(u16 *)(r10 -56) = r2
78: (bf) r2 = r1
79: (77) r2 >>= 32
80: (6b) *(u16 *)(r10 -58) = r2
81: (6b) *(u16 *)(r10 -62) = r1
82: (77) r1 >>= 16
83: (6b) *(u16 *)(r10 -60) = r1
84: (79) r1 = *(u64 *)(r10 -80)
85: (bf) r2 = r1
86: (77) r2 >>= 48
87: (6b) *(u16 *)(r10 -64) = r2
88: (bf) r2 = r1
89: (77) r2 >>= 32
90: (6b) *(u16 *)(r10 -66) = r2
91: (6b) *(u16 *)(r10 -70) = r1
92: (77) r1 >>= 16
93: (6b) *(u16 *)(r10 -68) = r1
94: (79) r1 = *(u64 *)(r10 -88)
95: (bf) r2 = r1
96: (77) r2 >>= 48
97: (6b) *(u16 *)(r10 -72) = r2
98: (bf) r2 = r1
99: (77) r2 >>= 32
100: (6b) *(u16 *)(r10 -74) = r2
101: (6b) *(u16 *)(r10 -78) = r1
102: (77) r1 >>= 16
103: (6b) *(u16 *)(r10 -76) = r1
104: (79) r1 = *(u64 *)(r10 -96)
105: (bf) r2 = r1
106: (77) r2 >>= 48
107: (6b) *(u16 *)(r10 -80) = r2
108: (bf) r2 = r1
109: (77) r2 >>= 32
110: (6b) *(u16 *)(r10 -82) = r2
111: (6b) *(u16 *)(r10 -86) = r1
112: (77) r1 >>= 16
113: (6b) *(u16 *)(r10 -84) = r1
114: (6b) *(u16 *)(r10 -88) = r6
115: (b7) r1 = 1
116: (7b) *(u64 *)(r10 -8) = r1
117: (18) r1 = 0xffff97f337e71600
119: (bf) r2 = r10
120: (07) r2 += -96
121: (bf) r3 = r10
122: (07) r3 += -8
123: (b7) r4 = 0
124: (85) call bpf_map_update_elem#2
125: (b7) r0 = 0
126: (95) exit
processed 125 insns, stack depth 96
```

In both cases, `task->nsproxy->uts_ns->name.nodename` will be loaded at `(r10 --96)` on instruction 30, but in the second case `(r10 --96)` is overridden by instruction 33, so the problem is here:

```
25: (79) r3 = *(u64 *)(r10 -8)
26: (07) r3 += 69
27: (bf) r1 = r10
28: (07) r1 += -96
29: (b7) r2 = 65
30: (85) call bpf_probe_read#4
31: (18) r1 = 0x727473796d
33: (7b) *(u64 *)(r10 -96) = r1
```

LLVM is not respecting the lifetime of the `loaded` variable and is overriding it's location on the BPF stack.

A quick workaround for users is "be careful with the order of string members in structures", but the second program is completely valid and should work, so we should probably fix it upstream.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reproducing the Python BCC example with the listed LLVM, BCC, and kernel versions, then compare the correct and faulty BPF instruction listings when the struct members are swapped. Investigate LLVM's BPF stack allocation and variable-lifetime handling around BPF_probe_read and the constant string. Done means the loaded buffer is not overwritten and both struct layouts produce the expected output.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, linux
Domain
compilers, operating-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.