Skip to content

Commit

Permalink
memory: fix off-by-one in bounds check
Browse files Browse the repository at this point in the history
It previously wasn't possible to access the last byte of the memory segment.
Also add a u64 overflow check.

This wasn't caught in tests because this requires accessing a page-aligned
variable in its entirety. Add a test with a page-aligned array Variable.

Signed-off-by: Timo Beckers <[email protected]>
  • Loading branch information
ti-mo committed Feb 20, 2025
1 parent 8b4b96a commit 061b77f
Show file tree
Hide file tree
Showing 7 changed files with 21 additions and 4 deletions.
5 changes: 4 additions & 1 deletion memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,10 @@ func (mm *Memory) ReadOnly() bool {

// bounds returns true if an access at off of the given size is within bounds.
func (mm *Memory) bounds(off uint64, size uint64) bool {
return off+size < uint64(len(mm.b))
if off+size < off {
return false
}
return off+size <= uint64(len(mm.b))
}

// ReadAt implements [io.ReaderAt]. Useful for creating a new [io.OffsetWriter].
Expand Down
6 changes: 3 additions & 3 deletions memory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ebpf

import (
"io"
"math"
"os"
"runtime"
"testing"
Expand Down Expand Up @@ -59,8 +60,7 @@ func TestMemoryBounds(t *testing.T) {
mm, err := mustMmapableArray(t, 0).Memory()
qt.Assert(t, qt.IsNil(err))

size := uint64(mm.Size())
end := size - 1
end := uint64(mm.Size())

qt.Assert(t, qt.IsTrue(mm.bounds(0, 0)))
qt.Assert(t, qt.IsTrue(mm.bounds(end, 0)))
Expand All @@ -69,7 +69,7 @@ func TestMemoryBounds(t *testing.T) {

qt.Assert(t, qt.IsFalse(mm.bounds(end-8, 9)))
qt.Assert(t, qt.IsFalse(mm.bounds(end, 1)))
qt.Assert(t, qt.IsFalse(mm.bounds(0, size)))
qt.Assert(t, qt.IsFalse(mm.bounds(math.MaxUint64, 1)))
}

func TestMemoryReadOnly(t *testing.T) {
Expand Down
Binary file modified testdata/variables-eb.elf
Binary file not shown.
Binary file modified testdata/variables-el.elf
Binary file not shown.
4 changes: 4 additions & 0 deletions testdata/variables.c
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,7 @@ volatile struct var_struct_t var_struct __section(".data.struct");
__section("socket") int check_struct() {
return var_struct.a == 0xa && var_struct.b == 0xb;
}

// Variable aligned on page boundary to ensure all bytes in the mapping can be
// accessed through the Variable API.
volatile char var_array[8192] __section(".data.array");
4 changes: 4 additions & 0 deletions variable.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ func (v *Variable) Set(in any) error {
return fmt.Errorf("variable %s: %w", v.name, ErrReadOnly)
}

if !v.mm.bounds(v.offset, v.size) {
return fmt.Errorf("variable %s: access out of bounds: %w", v.name, io.EOF)
}

buf, err := sysenc.Marshal(in, int(v.size))
if err != nil {
return fmt.Errorf("marshaling value %s: %w", v.name, err)
Expand Down
6 changes: 6 additions & 0 deletions variable_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ func TestVariable(t *testing.T) {
BSS *Variable `ebpf:"var_bss"`
Data *Variable `ebpf:"var_data"`
Struct *Variable `ebpf:"var_struct"`
Array *Variable `ebpf:"var_array"`
}{}

qt.Assert(t, qt.IsNil(spec.LoadAndAssign(&obj, nil)))
Expand All @@ -116,6 +117,11 @@ func TestVariable(t *testing.T) {
qt.Assert(t, qt.IsNil(obj.Struct.Set(&struct{ A, B uint64 }{0xa, 0xb})))
mustReturn(t, obj.CheckStruct, 1)

// Ensure page-aligned array variable can be accessed in its entirety.
arr := make([]byte, obj.Array.Size())
qt.Assert(t, qt.IsNil(obj.Array.Get(arr)))
qt.Assert(t, qt.IsNil(obj.Array.Set(arr)))

typ := obj.BSS.Type()
qt.Assert(t, qt.IsNotNil(typ))
i, ok := btf.As[*btf.Int](typ.Type)
Expand Down

0 comments on commit 061b77f

Please sign in to comment.