Skip to content

Commit

Permalink
Prevents panics during ELF parsing from crashing process (#30725)
Browse files Browse the repository at this point in the history
  • Loading branch information
brycekahle authored Nov 12, 2024
1 parent 5ed352e commit b0adcb9
Show file tree
Hide file tree
Showing 35 changed files with 354 additions and 157 deletions.
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@
/pkg/util/ecs/ @DataDog/container-integrations
/pkg/util/funcs/ @DataDog/ebpf-platform
/pkg/util/kernel/ @DataDog/ebpf-platform
/pkg/util/safeelf/ @DataDog/ebpf-platform
/pkg/util/ktime @DataDog/agent-security
/pkg/util/kubernetes/ @DataDog/container-integrations @DataDog/container-platform @DataDog/container-app
/pkg/util/podman/ @DataDog/container-integrations
Expand Down
2 changes: 2 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,8 @@ linters-settings:
desc: "Not really forbidden to use, but it is usually imported by mistake instead of github.com/stretchr/testify/assert"
- pkg: "github.com/tj/assert"
desc: "Not really forbidden to use, but it is usually imported by mistake instead of github.com/stretchr/testify/assert, and confusing since it actually has the behavior of github.com/stretchr/testify/require"
- pkg: "debug/elf"
desc: "prefer pkg/util/safeelf to prevent panics during parsing"

errcheck:
exclude-functions:
Expand Down
4 changes: 2 additions & 2 deletions pkg/collector/corechecks/servicediscovery/apm/detect.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ package apm

import (
"bufio"
"debug/elf"
"io"
"io/fs"
"os"
Expand All @@ -24,6 +23,7 @@ import (
"github.com/DataDog/datadog-agent/pkg/network/go/bininspect"
"github.com/DataDog/datadog-agent/pkg/util/kernel"
"github.com/DataDog/datadog-agent/pkg/util/log"
"github.com/DataDog/datadog-agent/pkg/util/safeelf"
)

// Instrumentation represents the state of APM instrumentation for a service.
Expand Down Expand Up @@ -100,7 +100,7 @@ const (
func goDetector(ctx usm.DetectionContext) Instrumentation {
exePath := kernel.HostProc(strconv.Itoa(ctx.Pid), "exe")

elfFile, err := elf.Open(exePath)
elfFile, err := safeelf.Open(exePath)
if err != nil {
log.Debugf("Unable to open exe %s: %v", exePath, err)
return None
Expand Down
7 changes: 3 additions & 4 deletions pkg/dynamicinstrumentation/diconfig/binary_inspection.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@
package diconfig

import (
"debug/elf"
"fmt"
"reflect"

"github.com/DataDog/datadog-agent/pkg/util/log"

"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/ditypes"
"github.com/DataDog/datadog-agent/pkg/network/go/bininspect"
"github.com/DataDog/datadog-agent/pkg/util/log"
"github.com/DataDog/datadog-agent/pkg/util/safeelf"
)

// inspectGoBinaries goes through each service and populates information about the binary
Expand Down Expand Up @@ -54,7 +53,7 @@ func AnalyzeBinary(procInfo *ditypes.ProcessInfo) error {

procInfo.TypeMap = typeMap

elfFile, err := elf.Open(procInfo.BinaryPath)
elfFile, err := safeelf.Open(procInfo.BinaryPath)
if err != nil {
return fmt.Errorf("could not open elf file %w", err)
}
Expand Down
5 changes: 3 additions & 2 deletions pkg/dynamicinstrumentation/diconfig/binary_inspection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
package diconfig

import (
"debug/elf"
"fmt"
"os"
"path/filepath"
Expand All @@ -18,6 +17,8 @@ import (

"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/testutil"
"github.com/DataDog/datadog-agent/pkg/network/go/bininspect"
"github.com/DataDog/datadog-agent/pkg/util/safeelf"

"github.com/kr/pretty"
)

Expand All @@ -40,7 +41,7 @@ func TestBinaryInspection(t *testing.T) {
t.Error(err)
}

f, err := elf.Open(binPath)
f, err := safeelf.Open(binPath)
if err != nil {
t.Error(err)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/dynamicinstrumentation/diconfig/config_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import (
"encoding/json"
"fmt"

"github.com/DataDog/datadog-agent/pkg/util/log"
"github.com/cilium/ebpf/ringbuf"
"github.com/google/uuid"

"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/codegen"
"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/diagnostics"
Expand All @@ -22,8 +23,7 @@ import (
"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/eventparser"
"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/proctracker"
"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/ratelimiter"
"github.com/cilium/ebpf/ringbuf"
"github.com/google/uuid"
"github.com/DataDog/datadog-agent/pkg/util/log"
)

type rcConfig struct {
Expand Down
8 changes: 4 additions & 4 deletions pkg/dynamicinstrumentation/diconfig/dwarf.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,16 @@ package diconfig
import (
"cmp"
"debug/dwarf"
"debug/elf"
"fmt"
"io"
"reflect"
"slices"

"github.com/DataDog/datadog-agent/pkg/util/log"
"github.com/go-delve/delve/pkg/dwarf/godwarf"

"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/ditypes"
"github.com/go-delve/delve/pkg/dwarf/godwarf"
"github.com/DataDog/datadog-agent/pkg/util/log"
"github.com/DataDog/datadog-agent/pkg/util/safeelf"
)

func getTypeMap(dwarfData *dwarf.Data, targetFunctions map[string]bool) (*ditypes.TypeMap, error) {
Expand Down Expand Up @@ -184,7 +184,7 @@ func loadDWARF(binaryPath string) (*dwarf.Data, error) {
if dwarfData, ok := dwarfMap[binaryPath]; ok {
return dwarfData, nil
}
elfFile, err := elf.Open(binaryPath)
elfFile, err := safeelf.Open(binaryPath)
if err != nil {
return nil, fmt.Errorf("couldn't open elf binary: %w", err)
}
Expand Down
11 changes: 5 additions & 6 deletions pkg/dynamicinstrumentation/proctracker/proctracker.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
package proctracker

import (
"debug/elf"
"errors"
"os"
"path/filepath"
Expand All @@ -19,19 +18,19 @@ import (
"sync"
"syscall"

"github.com/DataDog/datadog-agent/pkg/util/log"

"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/ditypes"
"github.com/cilium/ebpf"
"github.com/cilium/ebpf/link"
"golang.org/x/sys/unix"

"github.com/DataDog/datadog-agent/pkg/dynamicinstrumentation/ditypes"
"github.com/DataDog/datadog-agent/pkg/network/go/bininspect"
"github.com/DataDog/datadog-agent/pkg/network/go/binversion"
"github.com/DataDog/datadog-agent/pkg/process/monitor"
"github.com/DataDog/datadog-agent/pkg/security/secl/model"
"github.com/DataDog/datadog-agent/pkg/security/utils"
"github.com/DataDog/datadog-agent/pkg/util/kernel"
"golang.org/x/sys/unix"
"github.com/DataDog/datadog-agent/pkg/util/log"
"github.com/DataDog/datadog-agent/pkg/util/safeelf"
)

type processTrackerCallback func(ditypes.DIProcs)
Expand Down Expand Up @@ -134,7 +133,7 @@ func (pt *ProcessTracker) inspectBinary(exePath string, pid uint32) {
}
defer f.Close()

elfFile, err := elf.NewFile(f)
elfFile, err := safeelf.NewFile(f)
if err != nil {
log.Infof("file %s could not be parsed as an ELF file: %s", binPath, err)
return
Expand Down
10 changes: 5 additions & 5 deletions pkg/ebpf/uprobes/inspector.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
package uprobes

import (
"debug/elf"
"errors"
"fmt"
"runtime"
Expand All @@ -18,6 +17,7 @@ import (
"github.com/DataDog/datadog-agent/pkg/network/go/bininspect"
"github.com/DataDog/datadog-agent/pkg/network/usm/utils"
"github.com/DataDog/datadog-agent/pkg/util/common"
"github.com/DataDog/datadog-agent/pkg/util/safeelf"
)

// BinaryInspector implementors are responsible for extracting the metadata required to attach from a binary.
Expand Down Expand Up @@ -54,7 +54,7 @@ var _ BinaryInspector = &NativeBinaryInspector{}
// Inspect extracts the metadata required to attach to a binary from the ELF file at the given path.
func (p *NativeBinaryInspector) Inspect(fpath utils.FilePath, requests []SymbolRequest) (map[string]bininspect.FunctionMetadata, error) {
path := fpath.HostPath
elfFile, err := elf.Open(path)
elfFile, err := safeelf.Open(path)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -102,7 +102,7 @@ func (p *NativeBinaryInspector) Inspect(fpath utils.FilePath, requests []SymbolR
symbolMapBestEffort, _ := bininspect.GetAllSymbolsInSetByName(elfFile, bestEffortSymbols)

funcMap := make(map[string]bininspect.FunctionMetadata, len(symbolMap)+len(symbolMapBestEffort))
for _, symMap := range []map[string]elf.Symbol{symbolMap, symbolMapBestEffort} {
for _, symMap := range []map[string]safeelf.Symbol{symbolMap, symbolMapBestEffort} {
for symbolName, symbol := range symMap {
m, err := p.symbolToFuncMetadata(elfFile, symbol)
if err != nil {
Expand All @@ -115,8 +115,8 @@ func (p *NativeBinaryInspector) Inspect(fpath utils.FilePath, requests []SymbolR
return funcMap, nil
}

func (*NativeBinaryInspector) symbolToFuncMetadata(elfFile *elf.File, sym elf.Symbol) (*bininspect.FunctionMetadata, error) {
manager.SanitizeUprobeAddresses(elfFile, []elf.Symbol{sym})
func (*NativeBinaryInspector) symbolToFuncMetadata(elfFile *safeelf.File, sym safeelf.Symbol) (*bininspect.FunctionMetadata, error) {
manager.SanitizeUprobeAddresses(elfFile.File, []safeelf.Symbol{sym})
offset, err := bininspect.SymbolToOffset(elfFile, sym)
if err != nil {
return nil, err
Expand Down
31 changes: 6 additions & 25 deletions pkg/ebpf/verifier/elf.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,15 @@ package verifier

import (
"debug/dwarf"
"debug/elf"
"errors"
"fmt"
"io"
"path/filepath"
"runtime"

"github.com/cilium/ebpf"

"github.com/DataDog/datadog-agent/pkg/util/safeelf"
)

// getLineReader gets the line reader for a DWARF data object, searching in the compilation unit entry
Expand Down Expand Up @@ -133,7 +134,7 @@ func buildProgStartMap(dwarfData *dwarf.Data, symToSeq map[string]int) (map[prog
// buildSymbolToSequenceMap builds a map that links each symbol to the sequence index it belongs to.
// The address in the DWARF debug_line section is relative to the start of each sequence, but the symbol information
// doesn't explicitly say which sequence it belongs to. This function builds that map.
func buildSymbolToSequenceMap(elfFile *elf.File) (map[string]int, error) {
func buildSymbolToSequenceMap(elfFile *safeelf.File) (map[string]int, error) {
symbols, err := elfFile.Symbols()
if err != nil {
return nil, fmt.Errorf("failed to read symbols from ELF file: %w", err)
Expand All @@ -143,7 +144,7 @@ func buildSymbolToSequenceMap(elfFile *elf.File) (map[string]int, error) {
sectIndexToSeqIndex := make(map[int]int)
idx := 0
for i, sect := range elfFile.Sections {
if sect.Flags&elf.SHF_EXECINSTR != 0 && sect.Size > 0 {
if sect.Flags&safeelf.SHF_EXECINSTR != 0 && sect.Size > 0 {
sectIndexToSeqIndex[i] = idx
idx++
}
Expand All @@ -160,32 +161,12 @@ func buildSymbolToSequenceMap(elfFile *elf.File) (map[string]int, error) {
return symToSeq, nil
}

// openSafeELFFile opens an ELF file and recovers from panics that might happen when reading it.
func openSafeELFFile(path string) (safe *elf.File, err error) {
defer func() {
r := recover()
if r == nil {
return
}

safe = nil
err = fmt.Errorf("reading ELF file panicked: %s", r)
}()

file, err := elf.Open(path)
if err != nil {
return nil, err
}

return file, nil
}

// getSourceMap builds the source map for an eBPF program. It returns two maps, one that
// for each program function maps the instruction offset to the source line information, and
// another that for each section maps the functions that belong to it.
func getSourceMap(file string, spec *ebpf.CollectionSpec) (map[string]map[int]*SourceLine, map[string][]string, error) {
// Open the ELF file
elfFile, err := openSafeELFFile(file)
elfFile, err := safeelf.Open(file)
if err != nil {
return nil, nil, fmt.Errorf("cannot open ELF file %s: %w", file, err)
}
Expand All @@ -195,7 +176,7 @@ func getSourceMap(file string, spec *ebpf.CollectionSpec) (map[string]map[int]*S
// files because of missing support for relocations. However, we don't need them here as we're
// not necessary for line info, so we can skip them. The DWARF library will skip that processing
// if we set manually the type of the file to ET_EXEC.
elfFile.Type = elf.ET_EXEC
elfFile.Type = safeelf.ET_EXEC
dwarfData, err := elfFile.DWARF()
if err != nil {
return nil, nil, fmt.Errorf("cannot read DWARF data for %s: %w", file, err)
Expand Down
15 changes: 8 additions & 7 deletions pkg/gpu/cuda/cubin.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@ package cuda

import (
"bytes"
"debug/elf"
"encoding/binary"
"fmt"
"io"
"regexp"
"strings"

"github.com/DataDog/datadog-agent/pkg/util/safeelf"
)

// CubinKernelKey is the key to identify a kernel in a fatbin
Expand Down Expand Up @@ -126,7 +127,7 @@ type nvInfoItem struct {
Attr nvInfoAttr
}

type sectionParserFunc func(*elf.Section, string) error
type sectionParserFunc func(*safeelf.Section, string) error

// cubinParser is a helper struct to parse the cubin ELF sections
type cubinParser struct {
Expand Down Expand Up @@ -167,7 +168,7 @@ func (cp *cubinParser) parseCubinElf(data []byte) error {
}
data[elfVersionOffset] = 1

cubinElf, err := elf.NewFile(bytes.NewReader(data))
cubinElf, err := safeelf.NewFile(bytes.NewReader(data))
if err != nil {
return fmt.Errorf("failed to parse cubin ELF: %w", err)
}
Expand Down Expand Up @@ -203,7 +204,7 @@ type nvInfoParsedItem struct {
value []byte
}

func (cp *cubinParser) parseNvInfoSection(sect *elf.Section, kernelName string) error {
func (cp *cubinParser) parseNvInfoSection(sect *safeelf.Section, kernelName string) error {
items := make(map[nvInfoAttr]nvInfoParsedItem)
buffer := sect.Open()

Expand Down Expand Up @@ -253,7 +254,7 @@ func (cp *cubinParser) parseNvInfoSection(sect *elf.Section, kernelName string)
return nil
}

func (cp *cubinParser) parseTextSection(sect *elf.Section, kernelName string) error {
func (cp *cubinParser) parseTextSection(sect *safeelf.Section, kernelName string) error {
if kernelName == "" {
return nil
}
Expand All @@ -265,7 +266,7 @@ func (cp *cubinParser) parseTextSection(sect *elf.Section, kernelName string) er
return nil
}

func (cp *cubinParser) parseSharedMemSection(sect *elf.Section, kernelName string) error {
func (cp *cubinParser) parseSharedMemSection(sect *safeelf.Section, kernelName string) error {
if kernelName == "" {
return nil
}
Expand All @@ -278,7 +279,7 @@ func (cp *cubinParser) parseSharedMemSection(sect *elf.Section, kernelName strin

var constantSectNameRegex = regexp.MustCompile(`\.nv\.constant\d\.(.*)`)

func (cp *cubinParser) parseConstantMemSection(sect *elf.Section, _ string) error {
func (cp *cubinParser) parseConstantMemSection(sect *safeelf.Section, _ string) error {
// Constant memory sections are named .nv.constantX.Y where X is the constant memory index and Y is the name
// so we have to do some custom parsing
match := constantSectNameRegex.FindStringSubmatch(sect.Name)
Expand Down
Loading

0 comments on commit b0adcb9

Please sign in to comment.