Skip to content

Commit

Permalink
Changes interpreter data representation from Little to Big Endian.
Browse files Browse the repository at this point in the history 
Previously, it was thought that the interpreter stored data and did evaluations
in Little Endian. The confusion originated from the nft binary's debug output,
which is the primary artifact guiding the interpreter's implementation. The
debug output of the nft binary prints data in Little Endian although the data
is actually being stored in Big Endian. Since this is the only reference we
have to implement the interpreter, other than the Linux kernel, we thought the
data matched the debug output directly such that the interpreter's host
representation was Little Endian. The mistake became clear when looking at the
Linux kernel code for the byteorder operation and realizing the apparent
inconsistencies.

This also came with a few other changes as a result of switching endianness.
Allows data of any number of bytes (0,16]
Converts data in nftinterp from Little to Big Endian to account for the debug
output's transformation.

PiperOrigin-RevId: 668151185
  • Loading branch information
@gvisor-bot
Jayden Nyamiaka authored and gvisor-bot committed Aug 27, 2024
1 parent 218f52a commit 99745eb
Show file tree
Hide file tree
Showing 4 changed files with 128 additions and 93 deletions.
45 changes: 20 additions & 25 deletions pkg/tcpip/nftables/nftables.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ import (
"fmt"
"slices"

"encoding/binary"

"gvisor.dev/gvisor/pkg/abi/linux"
"gvisor.dev/gvisor/pkg/tcpip/stack"
)
Expand Down Expand Up @@ -658,22 +656,17 @@ func (op comparison) evaluate(regs *registerSet, pkt *stack.PacketBuffer) {
// Gets the data from the source register.
regBuf := bytesData.getRegisterBuffer(regs, op.sreg)

// Compares from left to right in 4-byte chunks starting with the rightmost
// byte of every 4-byte chunk since the data is little endian.
// For example, 16-byte IPv6 address 2001:000a:130f:0000:0000:09c0:876a:130b
// is represented as 0x0a000120 0x00000f13 0xc0090000 0x0b136a87 in operations
// and as [0a|00|01|20|00|00|0f|13|c0|09|00|00|0b|13|6a|87] in the byte slice,
// so we compare right to left in the first 4 bytes and then go to the next 4.
// Compares bytes from left to right for all bytes in the comparison data.
dif := 0
for i := 0; i < len(bytesData.data) && dif == 0; i += 4 {
regVal := binary.LittleEndian.Uint32(regBuf[i : i+4])
opVal := binary.LittleEndian.Uint32(bytesData.data[i : i+4])
if regVal < opVal {
for i := 0; i < len(bytesData.data) && dif == 0; i++ {
if regBuf[i] < bytesData.data[i] {
dif = -1
} else if regVal > opVal {
} else if regBuf[i] > bytesData.data[i] {
dif = 1
}
}

// Determines the comparison result depending on the operator.
var result bool
switch op.cop {
case linux.NFT_CMP_EQ:
Expand Down Expand Up @@ -777,22 +770,25 @@ func (rd verdictData) storeData(regs *registerSet, reg uint8) {
regs.verdict = rd.data
}

// bytesData represents data in 4-byte chunks to be stored in a register.
// bytesData represents <= 16 bytes of data to be stored in a register.
type bytesData struct {
data []byte
}

// newBytesData creates a RegisterData for 4, 8, 12, or 16 bytes of data.
// newBytesData creates a RegisterData for <= 16 bytes of data.
func newBytesData(bytes []byte) registerData {
if len(bytes)%4 != 0 || len(bytes) > 16 {
panic(fmt.Errorf("invalid byte data length: %d", len(bytes)))
if len(bytes) == 0 {
panic("bytes data cannot be empty")
}
if len(bytes) > 16 {
panic(fmt.Errorf("bytes data cannot be more than 16 bytes: %d", len(bytes)))
}
return bytesData{data: bytes}
}

// String returns a string representation of the bytes data.
// String returns a string representation of the big endian bytes data.
func (rd bytesData) String() string {
return fmt.Sprintf("%x", rd.data)
return fmt.Sprintf("be %x", rd.data)
}

// equal compares the bytes data to another RegisterData object.
Expand All @@ -812,7 +808,7 @@ func (rd bytesData) validateRegister(reg uint8) error {
if isVerdictRegister(reg) {
return fmt.Errorf("data cannot be stored in verdict register")
}
if is4ByteRegister(reg) && len(rd.data) != 4 {
if is4ByteRegister(reg) && len(rd.data) > 4 {
return fmt.Errorf("%d-byte data cannot be stored in 4-byte register", len(rd.data))
}
// 16-byte register can be used for any data (guaranteed to be <= 16 bytes)
Expand All @@ -824,15 +820,14 @@ func (rd bytesData) validateRegister(reg uint8) error {
// Note: does not support verdict data and assumes the register is valid for the
// given data type.
func (rd bytesData) getRegisterBuffer(regs *registerSet, reg uint8) []byte {
// The entire 4-byte register (data must be exactly 4 bytes)
// Returns the entire 4-byte register
if is4ByteRegister(reg) {
start := (reg - linux.NFT_REG32_00) * linux.NFT_REG32_SIZE
return regs.data[start : start+linux.NFT_REG32_SIZE]
}
// The appropriate (mod 4)-byte data in a 16-byte register
// Leaves excess space on the left (bc the data is little endian).
end := (int(reg)-linux.NFT_REG_1)*linux.NFT_REG_SIZE + linux.NFT_REG_SIZE
return regs.data[end-len(rd.data) : end]
// Returns the entire 16-byte register
start := (reg - linux.NFT_REG_1) * linux.NFT_REG_SIZE
return regs.data[start : start+linux.NFT_REG_SIZE]
}

// storeData sets the data in the destination register to the bytes data.
Expand Down
80 changes: 71 additions & 9 deletions pkg/tcpip/nftables/nftables_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,9 @@ func TestAcceptAllForSupportedHooks(t *testing.T) {
}
}

// TestEvaluateImmediate tests that the Immediate operation correctly sets the
// TestEvaluateImmediateVerdict tests that the Immediate operation correctly sets the
// register value and behaves as expected during evaluation.
func TestEvaluateImmediate(t *testing.T) {
func TestEvaluateImmediateVerdict(t *testing.T) {
for _, test := range []struct {
tname string
baseOp1 operation // will be nil if unused
Expand Down Expand Up @@ -234,6 +234,12 @@ func TestEvaluateImmediate(t *testing.T) {
baseOp2: mustCreateImmediate(t, linux.NFT_REG_VERDICT, newVerdictData(Verdict{Code: VC(linux.NF_ACCEPT)})),
verdict: Verdict{Code: VC(linux.NF_DROP)},
},
{
tname: "immediate load register",
baseOp1: mustCreateImmediate(t, linux.NFT_REG_VERDICT, newVerdictData(Verdict{Code: VC(linux.NF_DROP)})),
baseOp2: mustCreateImmediate(t, linux.NFT_REG_VERDICT, newVerdictData(Verdict{Code: VC(linux.NF_ACCEPT)})),
verdict: Verdict{Code: VC(linux.NF_DROP)},
},
} {
t.Run(test.tname, func(t *testing.T) {
// Sets up an NFTables object with a base chain (for 2 rules) and another
Expand Down Expand Up @@ -290,6 +296,62 @@ func TestEvaluateImmediate(t *testing.T) {
}
}

// TestEvaluateImmediateVerdict tests that the Immediate operation correctly
// loads bytes data of all lengths into all supported registers.
func TestEvaluateImmediateBytesData(t *testing.T) {
bytes := []byte{0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10}
for blen := 1; blen <= len(bytes); blen++ {
for _, registerSize := range []int{linux.NFT_REG32_SIZE, linux.NFT_REG_SIZE} {
if blen > registerSize {
continue
}
tname := fmt.Sprintf("immediately load %d bytes into %d-byte registers", blen, registerSize)
t.Run(tname, func(t *testing.T) {
// Sets up an NFTables object with a base chain with policy accept.
nf := NewNFTables()
tab, err := nf.AddTable(arbitraryFamily, "test", "test table", false)
if err != nil {
t.Fatalf("unexpected error for AddTable: %v", err)
}
bc, err := tab.AddChain("base_chain", nil, "test chain", false)
if err != nil {
t.Fatalf("unexpected error for AddChain: %v", err)
}
bc.SetBaseChainInfo(arbitraryInfoPolicyAccept)

// Adds a rule and immediate operation per register of registerSize.
switch registerSize {
case linux.NFT_REG32_SIZE:
for reg := linux.NFT_REG32_00; reg <= linux.NFT_REG32_15; reg++ {
rule := &Rule{}
rule.addOperation(mustCreateImmediate(t, uint8(reg), newBytesData(bytes[:blen])))
if err := bc.RegisterRule(rule, -1); err != nil {
t.Fatalf("unexpected error for RegisterRule for rule %d: %v", reg-linux.NFT_REG32_00, err)
}
}
case linux.NFT_REG_SIZE:
for reg := linux.NFT_REG_1; reg <= linux.NFT_REG_4; reg++ {
rule := &Rule{}
rule.addOperation(mustCreateImmediate(t, uint8(reg), newBytesData(bytes[:blen])))
if err := bc.RegisterRule(rule, -1); err != nil {
t.Fatalf("unexpected error for RegisterRule for rule %d: %v", reg-linux.NFT_REG_1, err)
}
}
}
// Runs evaluation and checks for default policy verdict accept
pkt := makeTestingPacket()
v, err := nf.EvaluateHook(arbitraryFamily, arbitraryHook, pkt)
if err != nil {
t.Fatalf("unexpected error for EvaluateHook: %v", err)
}
if v.Code != linux.NF_ACCEPT {
t.Fatalf("expected default policy verdict accept, got %v", v)
}
})
}
}
}

// TestEvaluateComparison tests that the Comparison operation correctly compares
// the data in the source register to the given data.
// Note: Relies on expected behavior of the Immediate operation.
Expand Down Expand Up @@ -345,8 +407,8 @@ func TestEvaluateComparison(t *testing.T) {
},
{
tname: "compare register > 4-byte data, true",
op1: mustCreateImmediate(t, linux.NFT_REG32_15, newBytesData([]byte{0, 0, 0, 1})),
op2: mustCreateComparison(t, linux.NFT_REG32_15, linux.NFT_CMP_GT, newBytesData([]byte{29, 76, 230, 0})),
op1: mustCreateImmediate(t, linux.NFT_REG32_15, newBytesData([]byte{29, 76, 230, 0})),
op2: mustCreateComparison(t, linux.NFT_REG32_15, linux.NFT_CMP_GT, newBytesData([]byte{0, 0, 0, 1})),
res: true,
},
{
Expand Down Expand Up @@ -381,8 +443,8 @@ func TestEvaluateComparison(t *testing.T) {
},
{
tname: "compare register >= 4-byte data, true gt",
op1: mustCreateImmediate(t, linux.NFT_REG32_12, newBytesData([]byte{0, 0, 0, 1})),
op2: mustCreateComparison(t, linux.NFT_REG32_12, linux.NFT_CMP_GTE, newBytesData([]byte{29, 76, 230, 0})),
op1: mustCreateImmediate(t, linux.NFT_REG32_12, newBytesData([]byte{29, 76, 230, 0})),
op2: mustCreateComparison(t, linux.NFT_REG32_12, linux.NFT_CMP_GTE, newBytesData([]byte{0, 0, 0, 1})),
res: true,
},
{
Expand Down Expand Up @@ -442,8 +504,8 @@ func TestEvaluateComparison(t *testing.T) {
},
{
tname: "compare register > 8-byte data, true",
op1: mustCreateImmediate(t, linux.NFT_REG_4, newBytesData([]byte{0, 0, 0, 1, 0, 0, 0, 0})),
op2: mustCreateComparison(t, linux.NFT_REG_4, linux.NFT_CMP_GT, newBytesData([]byte{29, 76, 230, 0, 0, 0, 0, 0})),
op1: mustCreateImmediate(t, linux.NFT_REG_4, newBytesData([]byte{29, 76, 230, 0, 0, 0, 0, 0})),
op2: mustCreateComparison(t, linux.NFT_REG_4, linux.NFT_CMP_GT, newBytesData([]byte{0, 0, 0, 1, 0, 0, 0, 0})),
res: true,
},
{
Expand Down Expand Up @@ -478,7 +540,7 @@ func TestEvaluateComparison(t *testing.T) {
},
{
tname: "compare register >= 8-byte data, true gt",
op1: mustCreateImmediate(t, linux.NFT_REG_2, newBytesData([]byte{0, 0, 0, 1, 0, 0, 0, 0})),
op1: mustCreateImmediate(t, linux.NFT_REG_2, newBytesData([]byte{30, 0, 0, 1, 0, 0, 0, 0})),
op2: mustCreateComparison(t, linux.NFT_REG_2, linux.NFT_CMP_GTE, newBytesData([]byte{29, 76, 230, 0, 0, 0, 0, 0})),
res: true,
},
Expand Down
13 changes: 8 additions & 5 deletions pkg/tcpip/nftables/nftinterp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,24 +365,27 @@ func parseVerdict(tokens []string, lnIdx int, tkIdx int) (int, Verdict, error) {
return tkIdx, v, nil
}

// parseHexData parses little endian hexadecimal data from the given token and
// returns the index of the next token to process (can consume multiple tokens).
// parseHexData parses little endian hexadecimal data from the given token,
// converts to big endian, and returns the index of the next token to process.
func parseHexData(tokens []string, lnIdx int, tkIdx int) (int, registerData, error) {
var bytes []byte
for ; tkIdx < len(tokens); tkIdx++ {
if len(tokens[tkIdx]) < 2 || tokens[tkIdx][:2] != "0x" {
if len(tokens[tkIdx]) <= 2 || tokens[tkIdx][:2] != "0x" {
break
}

if len(tokens[tkIdx]) != 10 {
return 0, nil, &SyntaxError{lnIdx, tkIdx, fmt.Sprintf("hexadecimal data must be exactly 8 digits long (excluding 0x): '%s'", tokens[tkIdx])}
// Hexadecimal data must have 2 digits per byte (even number of characters).
if len(tokens[tkIdx])%2 != 0 {
return 0, nil, &SyntaxError{lnIdx, tkIdx, fmt.Sprintf("invalid hexadecimal data: '%s'", tokens[tkIdx])}
}

// Decodes the little endian hex string into bytes
bytes4, err := hex.DecodeString(tokens[tkIdx][2:])
if err != nil {
return 0, nil, &SyntaxError{lnIdx, tkIdx, fmt.Sprintf("could not decode hexadecimal data: '%s'", tokens[tkIdx])}
}
// Converts the bytes to big endian and appends to the bytes slice.
slices.Reverse(bytes4)
bytes = append(bytes, bytes4...)
}
if len(bytes) > 16 {
Expand Down
Loading

0 comments on commit 99745eb

Please sign in to comment.