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.

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

x/sys/unix: FreeBSD PtraceIO violates unsafe.Pointer rules #58351

Closed
bcmills opened this issue Feb 6, 2023 · 4 comments
Closed

x/sys/unix: FreeBSD PtraceIO violates unsafe.Pointer rules #58351

bcmills opened this issue Feb 6, 2023 · 4 comments
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. OS-FreeBSD
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented Feb 6, 2023

The fix for #54113 appears to have been only partial.
GOOS=freebsd GOARCH=amd64 go vet ./... in the x/sys module shows a true-positive violation of the unsafe.Pointer rules in syscall_freebsd_amd64.go:

ioDesc := PtraceIoDesc{Op: int32(req), Offs: uintptr(unsafe.Pointer(addr)), Addr: uintptr(unsafe.Pointer(&out[0])), Len: uint64(countin)}
err = ptrace(PT_IO, pid, uintptr(unsafe.Pointer(&ioDesc)), 0)

Absent an API for pinning Go object addresses (#46787), this is incorrect and could lead to arbitrary memory corruption: if the out slice refers to memory in the Go heap, that memory can be collected (and reused for another allocation) concurrently with the call to ptrace.

(Note that the unsafe.Pointer rule that allows conversion of an unsafe.Pointer to a uintptr when calling syscall.Syscall applies only with the call expression itself, not within other variable declarations in the same function that makes the call.

This was apparently masked by #41205.

(attn @golang/freebsd; CC @tklauser @ianlancetaylor @aarzilli @mdempsky)
@bcmills bcmills added OS-FreeBSD NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 6, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 6, 2023
@gopherbot gopherbot added this to the Unreleased milestone Feb 6, 2023
@gopherbot
Copy link

Change https://go.dev/cl/465235 mentions this issue: all: avoid direct conversion of uintptr to unsafe.Pointer

@gopherbot
Copy link

Change https://go.dev/cl/465675 mentions this issue: unix: avoid converting non-pointers to unsafe.Pointer in PtraceIO

@gopherbot
Copy link

Change https://go.dev/cl/465676 mentions this issue: unix: mitigate uintptr violations in PtraceIO on freebsd

gopherbot pushed a commit to golang/sys that referenced this issue Feb 7, 2023
@gopherbot
unix: avoid converting non-pointers to unsafe.Pointer in PtraceIO
90c8f94 
Despite having the misleading type "void*" in the C API, the "offs"
field of the ptrace_io_desc struct is an offset within the child
process, and thus is not necessarily a valid pointer at all in the
parent process. The Go unsafe.Pointer type must refer only to valid
pointers, so converting this field through unsafe.Pointer is incorrect
and (in some cases) dangerous.

While we're here, let's also rename the "addr" function argument to
"offs", since that's the corresponding ptrace_io_desc field. It's very
confusing to have a function argument named "attr" that doesn't map to
the struct field of the same name!

For golang/go#58351.

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

bcmills commented Feb 7, 2023

(I believe this particular issue was introduced by the fix for #54113, but #58387 also applies to FreeBSD.)

aarzilli added a commit to aarzilli/delve that referenced this issue Feb 14, 2023
@aarzilli
proc/native: fix syscalls to SYS_PROCESS_VM_READV (and WRITEV)
95cd08b 
Per https://pkg.go.dev/unsafe#Pointer conversions from unsafe.Pointer
to uintptr are only safe in limited circumstances. In particular only
conversions made in the syscall call are pinned.
Additionally add a call to runtime.KeepAlive to mitigate the bug
described in: golang/go#58351
@aarzilli aarzilli mentioned this issue Feb 14, 2023
Merged
derekparker pushed a commit to go-delve/delve that referenced this issue Feb 14, 2023
@aarzilli
proc/native: fix syscalls to SYS_PROCESS_VM_READV (and WRITEV) (#3273)
95e33ed 
Per https://pkg.go.dev/unsafe#Pointer conversions from unsafe.Pointer
to uintptr are only safe in limited circumstances. In particular only
conversions made in the syscall call are pinned.
Additionally add a call to runtime.KeepAlive to mitigate the bug
described in: golang/go#58351
@golang golang locked and limited conversation to collaborators Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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. OS-FreeBSD
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
2 participants
@bcmills @gopherbot