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

x/net/bpf: bpf VM only supports running programs with big endian #20556

Open
mvo5 opened this issue Jun 2, 2017 · 10 comments
Open

x/net/bpf: bpf VM only supports running programs with big endian #20556

mvo5 opened this issue Jun 2, 2017 · 10 comments
Milestone

Comments

@mvo5
Copy link
Contributor

@mvo5 mvo5 commented Jun 2, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.7.4 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/egon/devel/go"
GORACE=""
GOROOT="/usr/lib/go-1.7"
GOTOOLDIR="/usr/lib/go-1.7/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build717134135=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

I am using x/net/bpf so that I can simulate a bunch of bpf programs generated by libseccomp-golang in the net/bpf VM to validate a small txt->bpf compiler.

I ran into the problem that the x/net/bpf VM assumes BigEndian byte order. However libseccomp-golang generates code in host byteorder (which happens to be little endian on my machine) so the simulation is not working as expected.

What did you expect to see?

I was hoping the VM would let me control the endianness. Please consider the following diff, it is fully backwards compatible but it will let me run libseccomp generated bpf programs inside the VM.

diff --git a/bpf/vm_instructions.go b/bpf/vm_instructions.go
index 516f946..2263c3f 100644
--- a/bpf/vm_instructions.go
+++ b/bpf/vm_instructions.go
@@ -134,6 +134,10 @@ func inBounds(inLen int, offset int, size int) bool {
        return offset+size <= inLen
 }
 
+// VmEndianness controlls what byte order the bpf.VM is using.
+// The default is binary.BigEndian
+var VmEndianness binary.ByteOrder = binary.BigEndian
+
 func loadCommon(in []byte, offset int, size int) (uint32, bool) {
        if !inBounds(len(in), offset, size) {
                return 0, false
@@ -143,9 +147,9 @@ func loadCommon(in []byte, offset int, size int) (uint32, bool) {
        case 1:
                return uint32(in[offset]), true
        case 2:
-               return uint32(binary.BigEndian.Uint16(in[offset : offset+size])), true
+               return uint32(VmEndianness.Uint16(in[offset : offset+size])), true
        case 4:
-               return uint32(binary.BigEndian.Uint32(in[offset : offset+size])), true
+               return uint32(VmEndianness.Uint32(in[offset : offset+size])), true
        default:
                panic(fmt.Sprintf("invalid load size: %d", size))
        }
@quentinmit
Copy link
Contributor

@quentinmit quentinmit commented Jun 2, 2017

We cannot review patches on GitHub. Please upload your patch to Gerrit by following https://golang.org/doc/contribute.html

@mdlayher
Copy link
Member

@mdlayher mdlayher commented Jun 2, 2017

I am certainly open to making the endianness configurable (probably via the VM's constructor). Please submit a CL and add me as a reviewer.

@mikioh
Copy link
Contributor

@mikioh mikioh commented Jun 2, 2017

Please don't expose a control knob like VMEndianness mentioned above, instead please keep the API surface machine endianness agnostic. See the discussion https://groups.google.com/d/msg/golang-nuts/3GEzwKfRRQw/O0Wr8A5q5SAJ for further information.

@mikioh mikioh changed the title [patch] x/net/bpf: bpf VM only supports running programs with big endian x/net/bpf: bpf VM only supports running programs with big endian Jun 2, 2017
@gopherbot gopherbot added this to the Unreleased milestone Jun 2, 2017
@mikioh
Copy link
Contributor

@mikioh mikioh commented Jun 2, 2017

@danderson
Copy link

@danderson danderson commented Jun 2, 2017

@mikioh Hmm, this is interesting. I'm not actually sure where the bug is here.

The BPF spec does not mention byte ordering at all. However, the VM supports multi-byte integer load instructions, so it's implicitly assuming something about byte ordering.

Empirically, when we use BPF for packet filtering in the kernel, the load operations use big-endian ordering. I know this because go.universe.tf/netboot/dhcp4 implements a filter on UDP dport, which is 2 bytes in network byte order, and in the compare instruction I just compare with a number in native byte order, and the compare behaves as expected. Therefore, in-kernel, the integer load instructions are big-endian.

The next thing we need to know is: what is the behavior of BPF when it's running within seccomp? The input data with seccomp is a kernel struct that contains syscall information, so it's logical to assume that integers in that payload are native-endian... But does that mean the kernel BPF VM is loading ints differently when running in seccomp?!

Once we know what the empirical behavior of seccomp-bpf is inside the kernel, we'll know what to fix: either the userspace VM implementation in this package, or libseccomp-golang, or maybe neither.

@mvo5 How well do you know seccomp-bpf? Do you know how the kernel implements integer load ops in that subsystem?

I'll try to dig through the kernel source code tonight to find some answers... But we should find these answers before trying to patch anything :)

@danderson
Copy link

@danderson danderson commented Jun 2, 2017

And by the way, if the bug is actually that the kernel implementation uses different byte ordering modes between packet filtering and seccomp, then my preferred fix would be to break the API of NewVM, and add a vmType argument that is an enum describing the different flavors of VM, instead of explicitly configuring endianness behavior.

So, for example, if you want a packet filter, you would write newVM(program, bpf.VMPacketFilter), and if you want seccomp you would write newVM(program, bpf.VMSyscallFilter), or something like that. That way, we keep the API focused on what the caller is doing with BPF programs, instead of forcing them to know exactly how to configure a VM for their specific use case.

Thoughts?

@mikioh
Copy link
Contributor

@mikioh mikioh commented Jun 3, 2017

I think it's fine to change the signature of NewVM for specifying VM flavors.

@mvo5
Copy link
Contributor Author

@mvo5 mvo5 commented Jun 3, 2017

@danderson Thanks a lot for looking into this issue! I'm fine with your suggestion to not expose the endianness directly. I'm also not deeply familar with the seccomp-bpf implementation but I did do poke around the code in libseccomp and it has explicit code to convert host integers to the targets endianess and the scmp_bpf_sim.c bpf simulator in the libseccomp code also has code that ensures the data is in the target arches native endianess (c.f. https://github.com/seccomp/libseccomp/blob/master/tools/scmp_bpf_sim.c#L321). I have not looked at the kernel seccomp-bpf code though.

mvo5 added a commit to mvo5/snappy that referenced this issue Jun 6, 2017
@mvo5
Copy link
Contributor Author

@mvo5 mvo5 commented Jun 6, 2017

@danderson I had a quick look at the kernel seccomp bpf code and the code states: Endianness is explicitly ignored and left for BPF program authors to manage as per the specific architecture (c.f. https://github.com/torvalds/linux/blob/master/kernel/seccomp.c#L68).

@mvo5
Copy link
Contributor Author

@mvo5 mvo5 commented Jun 6, 2017

I am currently sorting the CLA out, once I have that, I will propose a proper branch. In the meantime here is a diff is someone wants to check if the direction makes sense or give some early feedback: 0001-Add-bpf.VMType-and-modify-VM.NewVM-to-take-it.patch.gz.

mvo5 added a commit to mvo5/snappy that referenced this issue Jun 21, 2017
mvo5 added a commit to mvo5/snappy that referenced this issue Aug 22, 2017
The bpf of seccomp uses native endian. The x/net/bpf always uses
the architecture specific endian. This means we can not currently
simulate our geneated bpf with the bpf.VM. There is a open bug
at golang/go#20556

Given that we now also test the generated bpf against the in-kernel
seccomp implementation we can retire the bpf.VM tests (which test
exactly the same).
mvo5 added a commit to mvo5/snappy that referenced this issue Aug 22, 2017
The bpf code of seccomp uses native endian. The x/net/bpf VM
always uses the network endian. This means we can not currently
simulate our geneated bpf with the bpf.VM. There is a open bug
at golang/go#20556

Given that we now also test the generated bpf against the in-kernel
seccomp implementation we can retire the bpf.VM tests (which test
exactly the same).
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

6 participants