x/sys: invalid use of unsafe.Pointer in ioctl arg parameters #44834

brunnre8 · 2021-03-06T16:37:27Z

From the documentation of unsafe.Pointer:

(4) Conversion of a Pointer to a uintptr when calling syscall.Syscall.

The Syscall functions in package syscall pass their uintptr arguments
directly to the operating system, which then may, depending on the details
of the call, reinterpret some of them as pointers.
That is, the system call implementation is implicitly converting certain
arguments back from uintptr to pointer.

If a pointer argument must be converted to uintptr for use as an argument,
that conversion must appear in the call expression itself

Now, as far as I can tell this forces packages to adhere to exactly that.

The code in x/sys clearly violates that rule.

in unix/ioctl.go there's

// IoctlSetPointerInt performs an ioctl operation which sets an
// integer value on fd, using the specified request number. The ioctl
// argument is called with a pointer to the integer value, rather than
// passing the integer value directly.
func IoctlSetPointerInt(fd int, req uint, value int) error {
        v := int32(value)
        return ioctl(fd, req, uintptr(unsafe.Pointer(&v)))
}

and the declaration of ioctl in zsyskall_linux.go:

func ioctl(fd int, req uint, arg uintptr) (err error) {
        _, _, e1 := Syscall(SYS_IOCTL, uintptr(fd), uintptr(req), uintptr(arg))
        if e1 != 0 {
                err = errnoErr(e1)
        }
        return
}

I've asked on the mailing list if that's valid usage of unsafe, @ianlancetaylor wasn't sure and told me to open an issue here for further discussion. (https://groups.google.com/g/golang-nuts/c/Ocpy8CKs7ZI/m/3ym8gnuBFgAJ)

The text was updated successfully, but these errors were encountered:

cuonglm · 2021-04-30T16:01:49Z

This is safe.

v will be kept alive through ioctl call.
The rule is only applied for pointer must be converted to uintptr, ioctl does uintptr(arg) where arg is an uintptr, not a pointer.

cuonglm · 2021-05-03T07:43:44Z

cc @mdempsky

mdempsky · 2021-05-03T08:42:42Z

I don't think that code is safe.

The ioctl function does not have any pragmas to mark it as a syscall function, so the compiler won't know to do anything special for it. So the call to ioctl could potentially grow the stack, and then the arg pointer would be stale pointing to old memory.

cuonglm · 2021-05-03T08:53:41Z

I don't think that code is safe.

The ioctl function does not have any pragmas to mark it as a syscall function, so the compiler won't know to do anything special for it. So the call to ioctl could potentially grow the stack, and then the arg pointer would be stale pointing to old memory.

Hmm, so, whether it violates unsafe pointer rule?

Maybe the right fix is passing in an unsafe.Pointer instead of an uintptr 🤔

cuonglm · 2021-05-03T09:03:34Z

Maybe the right fix is passing in an unsafe.Pointer instead of an uintptr 🤔

or using go:nosplit

mdempsky · 2021-05-03T17:45:36Z

Maybe the right fix is passing in an unsafe.Pointer instead of an uintptr

I think so. The Linux ioctl man page at least says that the third argument is always a pointer. (If there are any ioctls that do actually pass a scalar value directly instead of a pointer, they'll need a separate utility wrapper.)

ianlancetaylor · 2021-05-03T21:33:03Z

@mdlayher added IoctlSetInt for Linux in https://golang.org/cl/44009 for #20474. That suggests that Linux has some ioctls that don't take a pointer value for the third argument, although off hand I do not know what they are.

lhl2617 · 2021-05-09T16:42:33Z

@mdlayher added IoctlSetInt for Linux in https://golang.org/cl/44009 for #20474. That suggests that Linux has some ioctls that don't take a pointer value for the third argument, although off hand I do not know what they are.

From https://man7.org/linux/man-pages/man2/ioctl.2.html:

The third argument is an untyped pointer to memory

From https://docs.oracle.com/cd/E23824_01/html/821-1463/ioctl-2.html

The arg argument represents a third argument that has additional information that is needed by this specific device to perform the requested function. The data type of arg depends upon the particular control request, but it is either an int or a pointer to a device-specific data structure.

It thus seems like IoctlSetInt was made for solaris consistent with the description in #20474. It should never be used for linux.

lhl2617 · 2021-05-09T16:54:46Z

Relevant issue: #34684

gopherbot · 2021-08-09T19:53:47Z

Change https://golang.org/cl/340915 mentions this issue: unix: generate ioctlNew on Linux with unsafe.Pointer arg

The existing ioctl stubs for all UNIX-like platforms take a value of type uintptr for the arg parameter. However, arguments which are cast from unsafe.Pointer to uintptr technically violate the rules for package unsafe. unsafe only allows a conversion from unsafe.Pointer to uintptr directly within a call to Syscall. ioctl is used on all UNIX-like operating systems and each one will have to be updated accordingly where pointer arguments are passed to system calls. To remedy this on Linux, we generate a new function called ioctlPtr which takes a value of type unsafe.Pointer for arg. More operating systems can be updated in future CLs by folks who have access to those systems and can run the appropriate code generator. Updates golang/go#44834 Change-Id: Ia9424be424b3dba91bb44d3a7a12bfb2179f0d86 Reviewed-on: https://go-review.googlesource.com/c/sys/+/340915 Trust: Matt Layher <mdlayher@gmail.com> Trust: Bryan C. Mills <bcmills@google.com> Run-TryBot: Matt Layher <mdlayher@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>

bcmills · 2023-02-07T17:01:56Z

@mdempsky, @ianlancetaylor, @mdlayher: was this fixed by CL 340915, or is there more to do here?

mdlayher · 2023-02-07T17:08:13Z

I believe the reason I didn't close this before was because I only fixed it for Linux. Similar changes would have to be made for *BSD and etc.

thediveo · 2023-02-07T21:40:32Z

@mdlayher added IoctlSetInt for Linux in https://golang.org/cl/44009 for #20474. That suggests that Linux has some ioctls that don't take a pointer value for the third argument, although off hand I do not know what they are.

Here we are, FICLONE https://man7.org/linux/man-pages/man2/ioctl_ficlonerange.2.html ... which I didn't knew existed a few moments ago 😁

gopherbot · 2023-02-17T22:02:51Z

Change https://go.dev/cl/469315 mentions this issue: unix: add ioctlPtr with unsafe.Pointer arg on other unices

gopherbot · 2023-02-20T12:06:49Z

Change https://go.dev/cl/469675 mentions this issue: syscall: avoid passing Go pointers as uintptr in exec tests

This is a followup for CL 340915 that adds ioctlPtr for all other UNIX-like platforms. For golang/go#44834 Change-Id: I0ecf84e53f13e5a8da736b3ba7f643262596d23c Reviewed-on: https://go-review.googlesource.com/c/sys/+/469315 Reviewed-by: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Dmitri Goutnik <dgoutnik@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com>

gopherbot · 2023-02-21T16:54:08Z

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

Avoid passing Go pointers as uintptr in exec_unix_test.go by introducing syscall.IoctlPtr() which accepts arg as unsafe.Pointer. For #44834 Fixes #58609 Change-Id: I6d0ded023e5f3c9989783aee7075bb88100d9ec2 Reviewed-on: https://go-review.googlesource.com/c/go/+/469675 Run-TryBot: Dmitri Goutnik <dgoutnik@gmail.com> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> Auto-Submit: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>

Avoid passing Go pointers as uintptr in exec_unix_test.go by introducing syscall.IoctlPtr() which accepts arg as unsafe.Pointer. For golang#44834 Fixes golang#58609 Change-Id: I6d0ded023e5f3c9989783aee7075bb88100d9ec2 Reviewed-on: https://go-review.googlesource.com/c/go/+/469675 Run-TryBot: Dmitri Goutnik <dgoutnik@gmail.com> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> Auto-Submit: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>

gopherbot · 2023-02-24T19:00:46Z

Change https://go.dev/cl/471119 mentions this issue: unix: add ioctlPtr with unsafe.Pointer arg on other unices (cont)

CL 469315 missed a few conversions, this CL adds them. While here, also update syscall wrapper generators. For golang/go#44834 Change-Id: I4418a8c177ee6d1a269c1cc2c806b199dc7ccf0b Reviewed-on: https://go-review.googlesource.com/c/sys/+/471119 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Dmitri Goutnik <dgoutnik@gmail.com> Reviewed-by: Bryan Mills <bcmills@google.com>

bcmills · 2023-05-03T19:53:37Z

@dmgk, can this issue be closed now, or is there more work remaining for it?

dmgk · 2023-05-03T20:29:12Z

I believe this is done, closing.

brunnre8 · 2023-05-04T01:30:34Z

Thanks a lot to everyone involved

gopherbot added this to the Unreleased milestone Mar 6, 2021

ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 8, 2021

mdlayher changed the title ~~x/sys: potential invalid use of unsafe~~ x/sys: invalid use of unsafe.Pointer in ioctl arg parameters Aug 10, 2021

bcmills mentioned this issue Feb 7, 2023

x/sys/unix: xattr functions erroneously pass Go pointers as type uintptr on BSDs #58386

Open

bcmills mentioned this issue Feb 7, 2023

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

Closed

bcmills added the help wanted label Feb 7, 2023

dmgk mentioned this issue Feb 20, 2023

syscall: introduce IoctlPtr for exec_unix tests #58609

Closed

dmgk closed this as completed May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/sys: invalid use of unsafe.Pointer in ioctl arg parameters #44834

x/sys: invalid use of unsafe.Pointer in ioctl arg parameters #44834

brunnre8 commented Mar 6, 2021

cuonglm commented Apr 30, 2021 •

edited

cuonglm commented May 3, 2021

mdempsky commented May 3, 2021

cuonglm commented May 3, 2021

cuonglm commented May 3, 2021

mdempsky commented May 3, 2021

ianlancetaylor commented May 3, 2021

lhl2617 commented May 9, 2021

lhl2617 commented May 9, 2021

gopherbot commented Aug 9, 2021

bcmills commented Feb 7, 2023

mdlayher commented Feb 7, 2023

thediveo commented Feb 7, 2023

gopherbot commented Feb 17, 2023

gopherbot commented Feb 20, 2023

gopherbot commented Feb 21, 2023

gopherbot commented Feb 24, 2023

bcmills commented May 3, 2023

dmgk commented May 3, 2023

brunnre8 commented May 4, 2023

x/sys: invalid use of unsafe.Pointer in ioctl arg parameters #44834

x/sys: invalid use of unsafe.Pointer in ioctl arg parameters #44834

Comments

brunnre8 commented Mar 6, 2021

cuonglm commented Apr 30, 2021 • edited

cuonglm commented May 3, 2021

mdempsky commented May 3, 2021

cuonglm commented May 3, 2021

cuonglm commented May 3, 2021

mdempsky commented May 3, 2021

ianlancetaylor commented May 3, 2021

lhl2617 commented May 9, 2021

lhl2617 commented May 9, 2021

gopherbot commented Aug 9, 2021

bcmills commented Feb 7, 2023

mdlayher commented Feb 7, 2023

thediveo commented Feb 7, 2023

gopherbot commented Feb 17, 2023

gopherbot commented Feb 20, 2023

gopherbot commented Feb 21, 2023

gopherbot commented Feb 24, 2023

bcmills commented May 3, 2023

dmgk commented May 3, 2023

brunnre8 commented May 4, 2023

cuonglm commented Apr 30, 2021 •

edited