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

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

Open
brunnre8 opened this issue Mar 6, 2021 · 10 comments
Open

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

brunnre8 opened this issue Mar 6, 2021 · 10 comments
Labels
NeedsInvestigation
Milestone

Comments

@brunnre8
Copy link

@brunnre8 brunnre8 commented Mar 6, 2021

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)

@gopherbot gopherbot added this to the Unreleased milestone Mar 6, 2021
@ianlancetaylor ianlancetaylor added the NeedsInvestigation label Mar 8, 2021
@cuonglm
Copy link
Member

@cuonglm cuonglm commented Apr 30, 2021

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
Copy link
Member

@cuonglm cuonglm commented May 3, 2021

cc @mdempsky

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 3, 2021

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
Copy link
Member

@cuonglm cuonglm commented May 3, 2021

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
Copy link
Member

@cuonglm cuonglm commented May 3, 2021

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

or using go:nosplit

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 3, 2021

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 3, 2021

@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
Copy link

@lhl2617 lhl2617 commented May 9, 2021

@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
Copy link

@lhl2617 lhl2617 commented May 9, 2021

Relevant issue: #34684

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 9, 2021

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

gopherbot pushed a commit to golang/sys that referenced this issue Aug 9, 2021
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>
@mdlayher 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

6 participants