diff --git a/pkg/tcpip/stack/iptables.go b/pkg/tcpip/stack/iptables.go index a9aba469d..2f2cfc438 100644 --- a/pkg/tcpip/stack/iptables.go +++ b/pkg/tcpip/stack/iptables.go @@ -315,7 +315,7 @@ func (it *IPTables) shouldSkipOrPopulateTables(tables []checkTable, pkt *PacketB // This is called in the hot path even when iptables are disabled, so we ensure // that it does not allocate. Note that called functions (e.g. // getConnAndUpdate) can allocate. -// TODO(b/233951539): checkescape fails on arm sometimes. Fix and re-add. +// +checkescape func (it *IPTables) CheckPrerouting(pkt *PacketBuffer, addressEP AddressableEndpoint, inNicName string) bool { tables := [...]checkTable{ { @@ -353,7 +353,7 @@ func (it *IPTables) CheckPrerouting(pkt *PacketBuffer, addressEP AddressableEndp // This is called in the hot path even when iptables are disabled, so we ensure // that it does not allocate. Note that called functions (e.g. // getConnAndUpdate) can allocate. -// TODO(b/233951539): checkescape fails on arm sometimes. Fix and re-add. +// +checkescape func (it *IPTables) CheckInput(pkt *PacketBuffer, inNicName string) bool { tables := [...]checkTable{ { @@ -393,7 +393,7 @@ func (it *IPTables) CheckInput(pkt *PacketBuffer, inNicName string) bool { // This is called in the hot path even when iptables are disabled, so we ensure // that it does not allocate. Note that called functions (e.g. // getConnAndUpdate) can allocate. -// TODO(b/233951539): checkescape fails on arm sometimes. Fix and re-add. +// +checkescape func (it *IPTables) CheckForward(pkt *PacketBuffer, inNicName, outNicName string) bool { tables := [...]checkTable{ { @@ -425,7 +425,7 @@ func (it *IPTables) CheckForward(pkt *PacketBuffer, inNicName, outNicName string // This is called in the hot path even when iptables are disabled, so we ensure // that it does not allocate. Note that called functions (e.g. // getConnAndUpdate) can allocate. -// TODO(b/233951539): checkescape fails on arm sometimes. Fix and re-add. +// +checkescape func (it *IPTables) CheckOutput(pkt *PacketBuffer, r *Route, outNicName string) bool { tables := [...]checkTable{ { @@ -467,7 +467,7 @@ func (it *IPTables) CheckOutput(pkt *PacketBuffer, r *Route, outNicName string) // This is called in the hot path even when iptables are disabled, so we ensure // that it does not allocate. Note that called functions (e.g. // getConnAndUpdate) can allocate. -// TODO(b/233951539): checkescape fails on arm sometimes. Fix and re-add. +// +checkescape func (it *IPTables) CheckPostrouting(pkt *PacketBuffer, r *Route, addressEP AddressableEndpoint, outNicName string) bool { tables := [...]checkTable{ { diff --git a/tools/checkescape/BUILD b/tools/checkescape/BUILD index 97909e78b..4dde541cf 100644 --- a/tools/checkescape/BUILD +++ b/tools/checkescape/BUILD @@ -4,7 +4,11 @@ package(licenses = ["notice"]) go_library( name = "checkescape", - srcs = ["checkescape.go"], + srcs = [ + "checkescape.go", + "checkescape_amd64.go", + "checkescape_arm64.go", + ], nogo = False, visibility = ["//tools/nogo:__subpackages__"], deps = [ diff --git a/tools/checkescape/checkescape.go b/tools/checkescape/checkescape.go index daaf7b835..c05673a8a 100644 --- a/tools/checkescape/checkescape.go +++ b/tools/checkescape/checkescape.go @@ -436,10 +436,11 @@ func loadObjdump(binary io.Reader) (finalResults map[string][]string, finalErr e } }() - // Identify calls by address or name. Note that this is also - // constructed dynamically below, as we encounted the addresses. - // This is because some of the functions (duffzero) may have - // jump targets in the middle of the function itself. + // Identify calls by address or name. Note that the list of allowed addresses + // -- not the list of allowed function names -- is also constructed + // dynamically below, as we encounter the addresses. This is because some of + // the functions (duffzero) may have jump targets in the middle of the + // function itself. funcsAllowed := map[string]struct{}{ "runtime.duffzero": {}, "runtime.duffcopy": {}, @@ -463,6 +464,8 @@ func loadObjdump(binary io.Reader) (finalResults map[string][]string, finalErr e "runtime.stackcheck": {}, "runtime.settls": {}, } + // addrsAllowed lists every address that can be jumped to within the + // funcsAllowed functions. addrsAllowed := make(map[string]struct{}) // Build the map. @@ -477,7 +480,8 @@ NextLine: } fields := strings.Fields(line) - // Is this an "allowed" function definition? + // Is this an "allowed" function definition? If so, record every address of + // the function body. if len(fields) >= 2 && fields[0] == "TEXT" { nextFunc = strings.TrimSuffix(fields[1], "(SB)") if _, ok := funcsAllowed[nextFunc]; !ok { @@ -485,7 +489,8 @@ NextLine: } } if nextFunc != "" && len(fields) > 2 { - // Save the given address (in hex form, as it appears). + // We're inside an allowed function. Save the given address (in hex form, + // as it appears). addrsAllowed[fields[1]] = struct{}{} } @@ -503,6 +508,10 @@ NextLine: } site := fields[0] target := strings.TrimSuffix(fields[4], "(SB)") + target, err := fixOffset(fields, target) + if err != nil { + return nil, err + } // Ignore strings containing allowed functions. if _, ok := funcsAllowed[target]; ok { diff --git a/tools/checkescape/checkescape_amd64.go b/tools/checkescape/checkescape_amd64.go new file mode 100644 index 000000000..f1522e8ff --- /dev/null +++ b/tools/checkescape/checkescape_amd64.go @@ -0,0 +1,20 @@ +// Copyright 2022 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package checkescape + +// fixOffset does nothing on amd64. +func fixOffset(fields []string, target string) (string, error) { + return target, nil +} diff --git a/tools/checkescape/checkescape_arm64.go b/tools/checkescape/checkescape_arm64.go new file mode 100644 index 000000000..47d382dd4 --- /dev/null +++ b/tools/checkescape/checkescape_arm64.go @@ -0,0 +1,69 @@ +// Copyright 2022 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package checkescape + +import ( + "fmt" + "strconv" + "strings" +) + +// fixOffset accounts for the output of arm64 `go tool objdump`. Objdump gives +// the instruction offset rather than the byte offset. The offset confuses +// checkescape, as it looks like the branch is to random memory. +// +// When appropriate, we re-parse the instruction ourselves and return the +// correct offset. +func fixOffset(fields []string, target string) (string, error) { + // We're looking for a line that looks something like: + // iptables.go:320 0x211214 97f9b198 CALL -413288(PC) + // In this case, target is passed as -413288(PC). The byte offset of this + // instruction should be -1653152. + + // Make sure we're dealing with a PC offset. + if !strings.HasSuffix(target, "(PC)") { + return target, nil // Not a relative branch. + } + + // Examine the opcode to ensure it's a BL instruction. See the ARM + // documentation here: + // https://developer.arm.com/documentation/ddi0596/2020-12/Base-Instructions/BL--Branch-with-Link-?lang=en + const ( + opcodeBits = 0xfc000000 + blOpcode = 0x94000000 + ) + instr64, err := strconv.ParseUint(fields[2], 16, 32) + if err != nil { + return "", err + } + instr := uint32(instr64) + if instr&opcodeBits != blOpcode { + return target, nil // Not a BL. + } + // Per documentation, the offset is formed via: + // - Take the lower 26 bits + // - Append 2 zero bits (this is what objdump omits) + // - Sign extend out to 64 bits + offset := int64(int32(instr<<6) >> 4) + + // Parse the PC, removing the leading "0x". + pc, err := strconv.ParseUint(fields[1][len("0x"):], 16, 64) + if err != nil { + return "", err + } + // PC is always the next instruction. + pc += 8 + return fmt.Sprintf("0x%x", pc+uint64(offset)), nil +}