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.

Sign up for GitHub

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

bpf: Fix Prefix operator for matchBinaries #2718

Closed
wants to merge 2 commits into from
Closed

bpf: Fix Prefix operator for matchBinaries #2718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4caee0f
bpf: Fix Prefix operator for matchBinaries
anfedotoff Jul 23, 2024
ec359d8
Add TestKprobeMatchBinariesPrefixLargePath
anfedotoff Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion bpf/process/bpf_execve_event.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,9 @@ execve_send(void *ctx)
{
struct msg_execve_event *event;
struct execve_map_value *curr;
#ifdef __LARGE_BPF_PROG
struct execve_heap *heap;
#endif
struct msg_process *p;
__u32 zero = 0;
uint64_t size;
Expand Down Expand Up @@ -329,10 +332,19 @@ execve_send(void *ctx)
memset(&curr->bin, 0, sizeof(curr->bin));
#ifdef __LARGE_BPF_PROG
// read from proc exe stored at execve time
if (event->exe.len <= BINARY_PATH_MAX_LEN) {
if (event->exe.len <= BINARY_PATH_MAX_LEN && !event->exe.error) {
curr->bin.path_length = probe_read(curr->bin.path, event->exe.len, event->exe.off);
if (curr->bin.path_length == 0)
curr->bin.path_length = event->exe.len;
} else {
heap = map_lookup_elem(&execve_heap, &zero);
Copy link
Member

@mtardy mtardy Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I'm not sure it's really appropriate as a fix:

  • The current implementation is indeed limited to 256 bytes on exe read, but this could be increased (maybe that's what you want to actually do). This is indeed bad that it's failing open for something as Prefix.
  • Here you read execve_heap that contains the binary only because of the way we read args at tracepoint/sys_execve, I don't think there are any guarantees that this heap will actually contain this btw since it's just a heap, we can use it for anything. This rollback to the old situation where we retrieved the filename from that args (which is still in place for <5.4) that could lead to read a relative path or a symlink (we switched to exe for that reason, reading the syscall args wasn't reliable). See:

    tetragon/bpf/process/bpf_execve_event.c

    Lines 248 to 254 in 92a17c2

    #ifdef __LARGE_BPF_PROG
    // Reading the absolute path of the process exe for matchBinaries.
    // Historically we used the filename, a potentially relative path (maybe to
    // a symlink) coming from the execve tracepoint. For kernels not supporting
    // large BPF prog, we still use the filename.
    read_exe((struct task_struct *)get_current_task(), &event->exe);
    #endif

I agree that there is a limitation on the current implementation which is pretty explicit (with BINARY_PATH_MAX_LEN), but falling back to the way we do things on <5.4 kernels will not help as it's also flawed and can be escaped with relative paths symlinks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking a bit more about this, it's indeed not great that we never check for the error at read from #1926 and that fallback to the args' filename could be an idea but it looks flawed. Indeed, we have unit tests on the function that uses read_exe and the error should be filled if the path is too big to enter the buffer of 256 (it seems that the limit is 255) so we should be aware that we could not match properly.

Because as your test highlights, an errored dentry will not work since it's reading from the end to the start, and thus the beginning of the path containing the prefix will not be here. Switching back to the old implem (as it seems you do even though you read from the execve_heap instead of p->args) could work until you don't use any relative path or symlink.

So I see two things:

  • support the error more explicitly and return an error to the user so that he knows we might have skipped an event because of path len limitation
  • fix the issue by increasing BINARY_PATH_MAX_LEN to 4096.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for detailed review! Yes you are right, relative paths and symlinks might be a problem. It is good to find a better solution that I proposed. Let me explain what I've done.

IIUC, we read the first argument here.

tetragon/bpf/process/bpf_execve_event.c

Lines 59 to 69 in 1a7b15a

/* skip first argument - binary path */
heap = map_lookup_elem(&execve_heap, &zero);
if (!heap)
return 0;
/* poor man's strlen */
off = probe_read_str(&heap->maxpath, 4096, (char *)start_stack);
if (off < 0)
return 0;
start_stack += off;

Here we think that p->args contains the first argument, but it seems to me that p->args contains arguments starting from the second one. So that's why I changed the logic for old kernels.

tetragon/bpf/process/bpf_execve_event.c

Line 338 in 1a7b15a

// reuse p->args first string that contains the filename, this can't be

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I see two things:
support the error more explicitly and return an error to the user so that he knows we might have skipped an event because of path len limitation
fix the issue by increasing BINARY_PATH_MAX_LEN to 4096.

I think it's a good idea to increase BINARY_PATH_MAX_LEN to 4096. If so we don't need file_copy_reverse here. And also, we don't need to have extra variables to hold postfix path.

Do you see any limitations that can stop us to increase the BINARY_PATH_MAX_LEN?
IIUC, limitations with prefix/postfix matching we can avoid by copying exact amount of data (STRING_PREFIX_MAX_LENGTH / STRING_POSTFIX_MAX_LENGTH).

Copy link
Contributor Author

@anfedotoff anfedotoff Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have problems here, if __LARGE_MAP_KEYS is not defined, I think:

tetragon/bpf/process/types/basic.h

Lines 1581 to 1587 in 1a7b15a

case op_filter_in:
case op_filter_notin:
path_map = map_lookup_elem(&tg_mb_paths, &selidx);
if (!path_map)
return 0;
found_key = map_lookup_elem(path_map, current->bin.path);
break;
.

But we can have a check for this case.

Copy link
Member

@mtardy mtardy Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we think that p->args contains the first argument, but it seems to me that p->args contains arguments starting from the second one. So that's why I changed the logic for old kernels.

It's not the case, event->process.args is read in function read_path and the first string separated by \0 is the first args, thus the (relative/symlink/etc) binary given to sys_execve:

tetragon/bpf/process/bpf_execve_event.c

Lines 102 to 111 in 8e07d6c

read_path(void *ctx, struct msg_execve_event *event, void *filename)
{
struct msg_process *p = &event->process;
__u32 size = 0;
__u32 flags = 0;
char *earg;
earg = (void *)p + offsetof(struct msg_process, args);
size = probe_read_str(earg, MAXARGLENGTH - 1, filename);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read_args, reads the args (hear the args of the execve binary not the args of the syscall), and thus use probe_read_str to remove the first part of the args, but does not touch it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I see, thanks making it clear!

Copy link
Member

@mtardy mtardy Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see any limitations that can stop us to increase the BINARY_PATH_MAX_LEN?

It all boils down to make prepend_name support more than 256 bytes. I'm looking into it at the moment as it's the only function I've written BPF unit tests so that might be helpful here.

if (heap) {
curr->bin.path_length = probe_read_str(curr->bin.path, BINARY_PATH_MAX_LEN, &heap->maxpath);
if (curr->bin.path_length > 1) {
// don't include the NULL byte in the length
curr->bin.path_length--;
}
}
}
#else
// reuse p->args first string that contains the filename, this can't be
Expand Down
42 changes: 42 additions & 0 deletions pkg/sensors/tracing/kprobe_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3886,6 +3886,48 @@ func TestKprobeMatchBinaries(t *testing.T) {
})
}

func TestKprobeMatchBinariesPrefixLargePath(t *testing.T) {
if !kernels.EnableLargeProgs() {
t.Skip()
}
repoBinPath := testutils.RepoRootPath("contrib/tester-progs/nop")
tmpDir := t.TempDir()

tmpDirLarge := tmpDir + "/" + strings.Repeat("a", 250)
err := os.Mkdir(tmpDirLarge, 0755)
assert.NoError(t, err)

tmpBinaryPath := tmpDirLarge + "/nop"

err = exec.Command("cp", repoBinPath, tmpBinaryPath).Run()
assert.NoError(t, err)

var doneWG, readyWG sync.WaitGroup
defer doneWG.Wait()

ctx, cancel := context.WithTimeout(context.Background(), tus.Conf().CmdWaitTime)
defer cancel()

createCrdFile(t, getMatchBinariesCrd("Prefix", []string{tmpDir}))

obs, err := observertesthelper.GetDefaultObserverWithFile(t, ctx, testConfigFile, tus.Conf().TetragonLib, observertesthelper.WithMyPid())
if err != nil {
t.Fatalf("GetDefaultObserverWithFile error: %s", err)
}
observertesthelper.LoopEvents(ctx, t, &doneWG, &readyWG, obs)
readyWG.Wait()

if err := exec.Command(tmpBinaryPath).Run(); err != nil {
t.Fatalf("failed to run nop: %s", err)
}

checker := ec.NewUnorderedEventChecker(ec.NewProcessKprobeChecker("").
WithProcess(ec.NewProcessChecker().WithBinary(sm.Full(tmpBinaryPath))).
WithFunctionName(sm.Full("fd_install")))
err = jsonchecker.JsonTestCheck(t, checker)
assert.NoError(t, err)
}

// matchBinariesPerfringTest checks that the matchBinaries do correctly
// filter the events i.e. it checks that no other events appear.
func matchBinariesPerfringTest(t *testing.T, operator string, values []string) {
Expand Down
Loading