Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

several race conditions in eBPF program #426

Open
evilsocket opened this issue May 27, 2021 · 7 comments
Open

several race conditions in eBPF program #426

evilsocket opened this issue May 27, 2021 · 7 comments

Comments

@evilsocket
Copy link
Owner

I believe that increments like this one should be done via:

__sync_fetch_and_add(value, 1)

according to this:

Since the defined array map is global, the accounting needs to use an atomic operation, which is defined as lock_xadd(). LLVM maps __sync_fetch_and_add() as a built-in function to the BPF atomic add instruction, that is, BPF_STX | BPF_XADD | BPF_W for word sizes.

i'm not an eBPF expert so before moving forward with the fixes i'd like to hear from @gustavo-iniguez-goya and @themighty1

@themighty1
Copy link
Contributor

Hi, @evilsocket , I think you are right, it might be racy.
However, before making any changes, one need to make sure that bpf in older kernels like 4.4 supports this atomic instruction.

@evilsocket
Copy link
Owner Author

i'm not even sure that the precompiled eBPF program would run on anything but 5.x ( see #427 ) ... anyways, that primitive is translated to

BPF_STX | BPF_XADD | BPF_W

and i believe these opcodes are there since eBPF was there ... not sure

@gustavo-iniguez-goya
Copy link
Collaborator

tested on 4.12.14, 4.15.0, 4.18, 5.4.0, 5.8.4 and 5.10.x (and 4.19 (i386)). On 4.9 (debian9) fails with error while loading kprobe/tcp_v4_connect

@evilsocket
Copy link
Owner Author

@gustavo-iniguez-goya what's the grep BPF /boot/config-$(uname -r) for that debian?

@gustavo-iniguez-goya
Copy link
Collaborator

CONFIG_BPF=y
CONFIG_BPF_SYSCALL=y
CONFIG_BPF_JIT=y
CONFIG_BPF_EVENTS=y

CONFIG_KPROBES=y
CONFIG_KPROBE_EVENT=y

@gustavo-iniguez-goya
Copy link
Collaborator

On the other hand, I've realized that we fail to enable this method on Debian Buster, because for some reason IPv6 established sockets can not be dumped via netlink:

eBPF could not dump TCPv6 sockets via netlink: Warning, no message nor error from netlink
eBPF error in dumping TCPv6 sockets via netlink

we fail here: https://github.com/evilsocket/opensnitch/blob/master/daemon/procmon/ebpf/ebpf.go#L140 . Letting it continue without returning there, ebpf works as expected.

Some systems/users disable IPv6, and I added a check (IPv6Enabled = Exists("/proc/sys/net/ipv6")) to verify this scenario, but on this system that directory is populated, so there must be something else going on there.

But maybe the problem is that there're no ipv6 connections... no idea, I'll investigate it.

@gustavo-iniguez-goya
Copy link
Collaborator

gustavo-iniguez-goya commented May 28, 2021

But maybe the problem is that there're no ipv6 connections... no idea, I'll investigate it.

ok, that's the problem, no TCP IPv6 connections, so we can ignore that error and keep working if eBPF doesn't fail to load.

e5b54f0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants