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

Jump to bottom

syscall,x/sys/unix: ptrace wrappers erroneously pass Go pointers as type uintptr #58387

Closed
bcmills opened this issue Feb 7, 2023 · 8 comments
Closed

syscall,x/sys/unix: ptrace wrappers erroneously pass Go pointers as type uintptr #58387

bcmills opened this issue Feb 7, 2023 · 8 comments
Assignees
@dmgk
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Feb 7, 2023

The unsafe.Pointer rules allow “conversion of a Pointer to a uintptr when calling syscall.Syscall”, with a caveat:

If a pointer argument must be converted to uintptr for use as an argument, that conversion must appear in the call expression itself:
…
The compiler handles a Pointer converted to a uintptr in the argument list of a call to a function implemented in assembly by arranging that the referenced allocated object, if any, is retained and not moved until the call completes, even though from the types alone it would appear that the object is no longer needed during the call.

The ptrace wrappers on both Linux and FreeBSD violate that requirement. They pass a uintptr argument to the ptrace helper function, which is what ultimately calls syscall.Syscall, and the arguments to the ptrace helper often do point to buffers allocated in Go:

These issues appear to date all the way back to 2009: the ptrace wrapper function was added in commit 9df5287 (CC @aclements), and in CL 126960043 which copied that pattern to x/sys.

The ptrace function likely needs a split like the one done for ioctl in #44834.

(attn @golang/runtime; CC @ianlancetaylor @bradfitz @tklauser)
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 7, 2023
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 7, 2023
@bcmills bcmills added this to the Backlog milestone Feb 7, 2023
@bcmills bcmills mentioned this issue Feb 7, 2023
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/465676 mentions this issue: unix: fix a use-after-free bug in PtraceIO on freebsd

gopherbot pushed a commit to golang/sys that referenced this issue Feb 8, 2023
@gopherbot
unix: fix a use-after-free bug in PtraceIO on freebsd
c79a742 
In CL 419915, both pointer fields of the PtraceIoDesc struct were
converted to type uintptr to address golang/go#54113.

However, that change was overzealous: the fix needed to convert fields
that refer to addresses in the child process, but the Addr field of
PtraceIoDesc is not even in the child process! It is instead an
address in the parent (Go) process.

Go's unsafe.Pointer rules prohibit converting a Go pointer to a
uintptr except when immediately converting back to an unsafe.Pointer
or calling a system call. Populating a PtraceIoDesc struct is neither
of those things, so converting the Addr field to uintptr introduced a
use-after-free bug.

This change reverts the change to the Addr field from CL 419915 and
consolidates the implementation of PtraceIO to reduce the the amount
of code that varies with GOARCH.

This change does not address the remaining ptrace uintptr bug
(golang/go#58387), which is also present in the Linux implementation.

Fixes golang/go#58351.
Updates golang/go#54113.
For golang/go#41205.

Change-Id: I14bdb4af42130aa7b4375e3f53fd1a0435f14307
Reviewed-on: https://go-review.googlesource.com/c/sys/+/465676
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@mknyszek
Copy link
Contributor

mknyszek commented Feb 8, 2023

In triage, @bcmills are you currently working on this? Assigned it to you for now but feel free to unassign.

@bcmills
Copy link
Contributor Author

bcmills commented Feb 8, 2023

I am not actively working on it, but I'm happy to advise and do code reviews.

(It should be straightforward plumbing, but will require regenerating and/or carefully editing generated files for a lot of different platforms)

@bcmills bcmills removed their assignment Feb 8, 2023
@dmgk dmgk self-assigned this Feb 21, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/469835 mentions this issue: unix: add ptracePtr that accepts pointer arg as unsafe.Pointer

@dmgk dmgk mentioned this issue Feb 21, 2023
Closed
gopherbot pushed a commit to golang/sys that referenced this issue Feb 21, 2023
@dmgk @gopherbot
unix: add ptracePtr that accepts pointer arg as unsafe.Pointer
2977c77 
The existing ptrace wrapper accepts pointer argument as an uintptr which
often points to the memory allocated in Go. This violates unsafe.Pointer safety
rules.

For golang/go#58387

Change-Id: Ib3b4c50368725191f0862c6c7c6d46b0568523c7
Reviewed-on: https://go-review.googlesource.com/c/sys/+/469835
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/470299 mentions this issue: syscall: add ptracePtr that accepts pointer arg as unsafe.Pointer

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/526455 mentions this issue: unix: remove unused ptracePtr on darwin

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/526475 mentions this issue: syscall: remove unused ptracePtr on darwin

gopherbot pushed a commit to golang/sys that referenced this issue Sep 7, 2023
@tklauser @gopherbot
unix: remove unused ptracePtr on darwin
bfd1ebb 
ptracePtr was introduced in CL 469835 for darwin but it's not used on
this platform. Also, the argument types for addr and data were swapped
in the generated ptrace1Ptr (probably because the change was not
generated but done manually).

This change also includes generated changes moving some definitions in
zsyscall_darwin_*.go and removing some empty lines in
zsyscall_darwin_*.s. These changes result from running ./mkall on
darwin/amd64 and darwin/arm64.

For golang/go#58387

Change-Id: I90bba3be7fbc8c77815450d0b666a2948b63fab0
Reviewed-on: https://go-review.googlesource.com/c/sys/+/526455
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Sep 7, 2023
@tklauser @gopherbot
syscall: remove unused ptracePtr on darwin
3a124de 
ptracePtr was introduced in CL 470299 for darwin but it's not used on
this platform. Also, the argument types for addr and data were swapped
in the generated ptrace1Ptr (probably because the change was not
generated but done manually).

For #58387

Change-Id: I429ab0c741e19020d98729c34efabce1d9003f56
Reviewed-on: https://go-review.googlesource.com/c/go/+/526475
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/527535 mentions this issue: syscall: remove unused ptracePtr on openbsd

gopherbot pushed a commit that referenced this issue Sep 14, 2023
@tklauser @gopherbot
syscall: remove unused ptracePtr on openbsd
630fff7 
ptracePtr was introduced in CL 470299 for openbsd but it's not used on
this platform.

For #58387

Change-Id: I27901c3f4497e18dc407a9248b66e2691c9a6abb
Reviewed-on: https://go-review.googlesource.com/c/go/+/527535
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gabyhelp gabyhelp mentioned this issue Jul 4, 2024
Closed
@golang golang locked and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@dmgk dmgk

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@mknyszek @bcmills @dmgk @gopherbot