Skip to content

feat(uprobes): add Go string parsing and inline modification#4817

Open
dwindsor wants to merge 8 commits into
cilium:mainfrom
dwindsor:dwindsor/clear-go-string-action
Open

feat(uprobes): add Go string parsing and inline modification#4817
dwindsor wants to merge 8 commits into
cilium:mainfrom
dwindsor:dwindsor/clear-go-string-action

Conversation

@dwindsor
Copy link
Copy Markdown
Contributor

@dwindsor dwindsor commented Mar 31, 2026

Fixes #4827

Description

With the pclntab changes being merged, we now can write policies that target stripped Go binaries. The problem is, pclntab doesn't contain function signature metadata, only function location data (offset + size).

This series adds support for the go_string type and the clearGoString action to Override in TracingPolicy. It requires a fairly complicated descent into the Go ABI internals, but I think the ABI has been stable enough for a while (since 1.18) to make this safe. Basically, since we have the function start location from pclntab, we just need to be able to isolate go string parameters, which are stored as a ([word][u32]) tuple (data_ptr+len). The new Go ABI (ABIInternal) is register based, with predictable slot assignments for types that haven't changed since 1.18. We go:generate the ABI slot mapping at build time, so no hard-coding.

The following figure illustrates how Go datatypes map to register slots on Go 1.18+ (ABIInternal):

ServeContent

 With this, we can use 7384893 (bpf: Allow uprobe program to change context registers) to add the clearGoString action to go_string parameters that contain suspicious (i.e. SQL injection, environment var leak, malicious AI prompt) strings.  We don't return early here, we just make the string inert by clearing it, allowing the application to handle cleanup etc. It's difficult to return from non-LSM functions (lsm gives a lot of guarantees - locks held, refcounts guaranteed, etc).

Screenshot 2026-04-22 at 4 56 53 PM

Changelog

@dwindsor dwindsor requested a review from a team as a code owner March 31, 2026 18:49
@dwindsor dwindsor requested a review from kevsecurity March 31, 2026 18:49
@dwindsor dwindsor force-pushed the dwindsor/clear-go-string-action branch 4 times, most recently from b6e596e to 08528fe Compare March 31, 2026 19:34
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 31, 2026

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 34d38ec
🔍 Latest deploy log https://app.netlify.com/projects/tetragon/deploys/6a01ee76108bde000863ca76
😎 Deploy Preview https://deploy-preview-4817--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@dwindsor dwindsor force-pushed the dwindsor/clear-go-string-action branch 2 times, most recently from 9893e14 to 276ab1c Compare March 31, 2026 19:40
@dwindsor dwindsor changed the title feat/uprobes: add Go string parsing and inline modification feat(uprobes): add Go string parsing and inline modification Mar 31, 2026
@dwindsor dwindsor force-pushed the dwindsor/clear-go-string-action branch 6 times, most recently from dacb94e to 1ecce4f Compare April 1, 2026 04:06
@dwindsor
Copy link
Copy Markdown
Contributor Author

dwindsor commented Apr 1, 2026

vmtests CI failing due to a verifier error on < 6.12, will fix:

❌ pkg.sensors.tracing.TestUprobeGoStringArg

@mtardy mtardy marked this pull request as draft April 3, 2026 16:01
@dwindsor dwindsor force-pushed the dwindsor/clear-go-string-action branch 6 times, most recently from d3f7dfa to 39ff357 Compare April 6, 2026 16:54
@dwindsor
Copy link
Copy Markdown
Contributor Author

dwindsor commented Apr 6, 2026

vmtests failing on kernels that don't have bpf_copy_from_user_str

424: (85) call unknown#233685214
invalid func unknown#233685214

Gating go_string support on the presence of bpf_copy_from_user_str.

@dwindsor dwindsor force-pushed the dwindsor/clear-go-string-action branch 5 times, most recently from f6985b1 to f20dd45 Compare April 6, 2026 17:19
@dwindsor dwindsor force-pushed the dwindsor/clear-go-string-action branch 2 times, most recently from 43a3f6c to 067c4e5 Compare April 20, 2026 17:25
@dwindsor dwindsor requested a review from olsajiri April 20, 2026 18:19
@dwindsor dwindsor marked this pull request as ready for review April 20, 2026 18:19
@dwindsor
Copy link
Copy Markdown
Contributor Author

dwindsor commented Apr 20, 2026

Thank you for the review @olsajiri! Comments mostly all addressed, a few I have questions on.

CI should be fine, just missing a pr label. Windows CI failing, unrelated:

Looking for regex: \{\"process_exec\"\:\{\"process\"\:\{\"exec_id\"\:\".{16,30}\"\,.{0,1}\"pid\"\:5772\,.{0,1}\"uid\"\:[0-9]{0,9}\,.{0,1}\"binary\"\:\"C:\\\\Windows\\\\system32\\\\notepad.exe\"
Get-Content: D:\a\_temp\f4d56df1-39bf-46f4-8c85-7f80bf400926.ps1:39
Line |
  39 |  $jsonContent = Get-Content -Path $jsonFilePath
     |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Cannot find path 'C:\Program Files\Tetragon\events.json' because it does not exist.
Error: Process completed with exit code 1.

@dwindsor dwindsor force-pushed the dwindsor/clear-go-string-action branch from 067c4e5 to 843bfa2 Compare April 21, 2026 14:32

err = jsonchecker.JsonTestCheck(t, checker)
require.NoError(t, err)
policytest.AllPolicyTests.DoObserverTest(t, "uprobe-go-string-arg", nil)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please squash this with the commit that adds this test

Copy link
Copy Markdown
Contributor Author

@dwindsor dwindsor Apr 23, 2026

Choose a reason for hiding this comment

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

done

Comment thread bpf/process/user_preload.h Outdated
len = sizeof(data->data) - 4;
asm volatile("%[len] &= 0xfff;\n"
: [len] "+r"(len));
err = probe_read_user(&data->data[4], len, (void *)ptr);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

probe_read_user does not allow faults and the point of preload is that we allow faults,
I think we need to use copy_from_user_task instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! copy_from_user_task does appear to allow faults (it runs with .might_sleep = true). Updated to use that.

}

if argType == gt.GenericGoStringType {
if !bpf.HasKfunc("bpf_copy_from_user_str") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this kfunc seems unrelated to the go_string preload

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is strange because the go_string preload indeed doesn't call bpf_copy_from_user_str, yet without this, vmtests fail in CI on kernels that are missing copy_from_user_str. I haven't quite figured out why that is the case yet. See this above comment: #4817 (comment)

if symbols == 0 {
return errors.New("go_string type requires at least one symbol")
}
sym := spec.Symbols[0]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what if there's more symbols in the uprobe spec? I think we should fail the spec in such case

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Updated to reject if the # of arguments isn't exactly 1.

return errors.New("error: can't preload more than one argument")
}
if !goABIValidated {
if err := f.ValidateGoABI(); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could this be called later underIsStrippedPureGoBinary condition?

        if symbols != 0 && f.IsStrippedPureGoBinary() {
                tbl, err := f.Pclntab()
                if err != nil {
                        return nil, fmt.Errorf("failed to parse pclntab: %w", err)
                }

                -> in here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not without the side effect of removing ABI validation on non-stripped binaries. It would be nice if this worked transparently for both cases.

Comment thread cmd/goabi-gen/main.go Outdated
case *types.Array:
return int(u.Len()) * intRegSlots(u.Elem())
}
return 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

curious.. looks like in here we should return 0, warn or error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. This is a leftover hack from when I was trying to get this working with non-stdlib types.

Returning error now when an unknown type is encountered. We don't want to generate a slot table that's based on guessed slot counts.

Added a unit test to explicitly test for this condition: slot generation for an unknown type.

Comment thread pkg/selectors/kernel.go Outdated
return errors.New("parseMatchAction: clearGoString requires uprobe symbol context")
}
var err error
argRegs, err = k.uprobeGoStringClearRegs(action.ArgIndex)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this seems worse to me.. I can't find your previous version, but I think now this is more complicated, sorry..

how about we leave user to configure action.ClearGoString and we validate that user did not provide argsRegs and then we populate argsRegs with the needed register assignment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's fine, I have no strong opinion on how the action gets translated to registers, I just would rather not allow the user to populate regs whenever it can be fixed up behind the scenes. We'll find a way that works =).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but aren't we already doing that?

We validate that they didn't provide argRegs:

if len(argRegs) > 0 {
	return errors.New("parseMatchAction: clearGoString cannot be combined with argRegs")
}

Immediately after we populate argRegs using uprobeGoStringClearRegs. I might be missing something you were thinking about though?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, but I think it's better to do it in uprobe code without theuprobeGoStringClearRegs callback ... just prepare argRegs in uprobe sensor code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see. Moved the args fixup code to expandClearGoStringActions and call that in addUprobe after elf.SafeOpenFile.


// GoABISlotRegNames returns the ptr and len register names for a Go string at slot.
func GoABISlotRegNames(slot int) (ptrReg, lenReg string, err error) {
if !archHasGoString() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it's bit of a shortcut now, but I guess full arch split can wait when we get another arch support

Copy link
Copy Markdown
Contributor Author

@dwindsor dwindsor Apr 22, 2026

Choose a reason for hiding this comment

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

Yeah other arch support will be tiny PRs compared to this.

The reason this PR is so massive is that it adds the Go ABI slot generation code and updates CRDs. I didn't want to add that in a separate PR without a user, hence we have it here in this PR with the most useful (in my opinion) type to write policies against (strings).

@dwindsor dwindsor force-pushed the dwindsor/clear-go-string-action branch 6 times, most recently from 0775599 to 3750c07 Compare April 27, 2026 16:01
@kkourt kkourt added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 29, 2026
@dwindsor dwindsor force-pushed the dwindsor/clear-go-string-action branch 4 times, most recently from d32c9e1 to a5bf1e5 Compare April 29, 2026 13:40
@kkourt kkourt added the release-note/major This PR introduces major new functionality label Apr 29, 2026
dwindsor added 8 commits May 11, 2026 10:57
Map logical argument indices to physical register slots under Go's
ABIInternal calling convention on amd64. Multi-slot types shift
subsequent arguments into higher registers. This is needed to extract
the correct argument from uprobes attached to Go functions.

Signed-off-by: David Windsor <dwindsor@gmail.com>
Cover GoABISlotForArg slot resolution for representative symbols.

Signed-off-by: David Windsor <dwindsor@gmail.com>
Wire the go_string generic type through BPF user preload and event
configuration, and expose it in the tracing API, generic types, ELF
pclntab resolution, selectors, and the uprobe sensor path.

Signed-off-by: David Windsor <dwindsor@gmail.com>
Bump CRD schema and docs for the new go_string type.

Signed-off-by: David Windsor <dwindsor@gmail.com>
Signed-off-by: David Windsor <dwindsor@gmail.com>
Add ClearGoString argument for Override that clears Go strings in userspace.

Strings are stored as [ptr,len] tuples in Go. We only zero len here
because it's possible to do atomically with a single aligned word-size write.
Zeroing the ptr field would require another write which cannot be performed
atomically with respect to the clearing of the len field.

With len cleared to zero the header matches how the runtime treats empty results
from []byte-to-string conversion: slicebytetostring returns "" when n==0
without reading ptr [1].

[1] Link: https://github.com/golang/go/blob/go1.24.0/src/runtime/string.go#L132-L137

Signed-off-by: David Windsor <dwindsor@gmail.com>
Cover ClearGoString match-action expansion into Override arg regs for
representative uprobes.

Signed-off-by: David Windsor <dwindsor@gmail.com>
Signed-off-by: David Windsor <dwindsor@gmail.com>
}

// GoABISlotRegNames returns the ptr and len register names for a Go string at slot.
func GoABISlotRegNames(slot int) (ptrReg, lenReg string, err error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit, no need for this to be global

shift = 64 - reg->size * 8;

val = read_reg(ctx, reg->offset, shift);
ty = config->arg[index];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I dont understand this change.. no nee dto move this right?

case string_type:
size = copy_strings(args, (char *)arg, MAX_STRING);
break;
case go_string_type: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need to add go_string_type to type_to_min_size

sel := &spec.Selectors[si]
for ai := range sel.MatchActions {
act := &sel.MatchActions[ai]
if !act.ClearGoString {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this looks better now, but should it also check there is actualy argument with go_string in the spec? otherwise the ValidateGoABI won't be called later on

err = copy_from_user_task(&data->data[4], len, (void *)ptr, task, 0);
if (err) {
*(__u32 *)data->data = 0;
data->status = (__u32)-err;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we expect (__u32) -1 as error.. anything else is interpreted as depth error during resolve (getArgStatus)

Comment thread bpf/include/api.h
static int BPF_FUNC(probe_read_user, void *dst, uint32_t size, const void *src);
static int BPF_FUNC(probe_write_user, void *dst, const void *src, uint32_t len);
static int BPF_FUNC(copy_from_user, void *dst, uint32_t size, const void *src);
struct task_struct;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this needed? I get clean build without it

Comment thread cmd/goabi-gen/main.go
@@ -0,0 +1,256 @@
// SPDX-License-Identifier: Apache-2.0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this seems to break make codegen:

...
go generate ./pkg/sensors/tracing/...
2026/05/14 09:35:16 write pkg/sensors/tracing/goabi_slots_gen.go: open pkg/sensors/tracing/goabi_slots_gen.go: no such file or directory
exit status 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase This PR needs to be rebased because it has merge conflicts. release-note/major This PR introduces major new functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to write policies against Go string function args

3 participants