From 04fa5793712fc4feea8c29107611b9b65cebbb61 Mon Sep 17 00:00:00 2001 From: Thomas Kohler Date: Sat, 11 Nov 2023 19:31:25 +0100 Subject: [PATCH] i2c(core): fix problems with usage of uintptr (#1033) --- drivers/i2c/i2c_connection_test.go | 8 +++-- system/i2c_device.go | 26 ++++++++++------ system/i2c_device_test.go | 50 ++++++++++++++++++------------ system/syscall.go | 26 ++++++++++++++-- system/syscall_mock.go | 17 +++++++--- system/system.go | 9 +++++- 6 files changed, 96 insertions(+), 40 deletions(-) diff --git a/drivers/i2c/i2c_connection_test.go b/drivers/i2c/i2c_connection_test.go index e4a610e95..92d075a57 100644 --- a/drivers/i2c/i2c_connection_test.go +++ b/drivers/i2c/i2c_connection_test.go @@ -14,18 +14,20 @@ import ( const dev = "/dev/i2c-1" -func getSyscallFuncImpl(errorMask byte) func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err system.SyscallErrno) { +func getSyscallFuncImpl( + errorMask byte, +) func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err system.SyscallErrno) { // bit 0: error on function query // bit 1: error on set address // bit 2: error on command - return func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err system.SyscallErrno) { + return func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err system.SyscallErrno) { // function query if (trap == system.Syscall_SYS_IOCTL) && (a2 == system.I2C_FUNCS) { if errorMask&0x01 == 0x01 { return 0, 0, 1 } - var funcPtr *uint64 = (*uint64)(unsafe.Pointer(a3)) + var funcPtr *uint64 = (*uint64)(a3) *funcPtr = system.I2C_FUNC_SMBUS_READ_BYTE | system.I2C_FUNC_SMBUS_READ_BYTE_DATA | system.I2C_FUNC_SMBUS_READ_WORD_DATA | system.I2C_FUNC_SMBUS_WRITE_BYTE | system.I2C_FUNC_SMBUS_WRITE_BYTE_DATA | diff --git a/system/i2c_device.go b/system/i2c_device.go index 2c2e4cba1..3b635e03d 100644 --- a/system/i2c_device.go +++ b/system/i2c_device.go @@ -160,7 +160,8 @@ func (d *i2cDevice) ReadBlockData(address int, reg uint8, data []byte) error { buf := make([]byte, dataLen+1) buf[0] = byte(dataLen) copy(buf[1:], data) - if err := d.smbusAccess(address, I2C_SMBUS_READ, reg, I2C_SMBUS_I2C_BLOCK_DATA, unsafe.Pointer(&buf[0])); err != nil { + if err := d.smbusAccess(address, I2C_SMBUS_READ, reg, I2C_SMBUS_I2C_BLOCK_DATA, + unsafe.Pointer(&buf[0])); err != nil { return err } // get data from buffer without first size element @@ -322,7 +323,7 @@ func (d *i2cDevice) read(address int, b []byte) (n int, err error) { func (d *i2cDevice) queryFunctionality(requested uint64, sender string) error { // lazy initialization if d.funcs == 0 { - if err := d.syscallIoctl(I2C_FUNCS, unsafe.Pointer(&d.funcs), "Querying functionality"); err != nil { + if err := d.syscallIoctl(I2C_FUNCS, unsafe.Pointer(&d.funcs), 0, "Querying functionality"); err != nil { return err } } @@ -334,7 +335,13 @@ func (d *i2cDevice) queryFunctionality(requested uint64, sender string) error { return nil } -func (d *i2cDevice) smbusAccess(address int, readWrite byte, command byte, protocol uint32, dataStart unsafe.Pointer) error { +func (d *i2cDevice) smbusAccess( + address int, + readWrite byte, + command byte, + protocol uint32, + dataStart unsafe.Pointer, +) error { if err := d.setAddress(address); err != nil { return err } @@ -346,8 +353,9 @@ func (d *i2cDevice) smbusAccess(address int, readWrite byte, command byte, proto data: dataStart, // the reflected value of unsafePointer equals uintptr(dataStart), } - sender := fmt.Sprintf("SMBus access r/w: %d, command: %d, protocol: %d, address: %d", readWrite, command, protocol, d.lastAddress) - if err := d.syscallIoctl(I2C_SMBUS, unsafe.Pointer(&smbus), sender); err != nil { + sender := fmt.Sprintf("SMBus access r/w: %d, command: %d, protocol: %d, address: %d", + readWrite, command, protocol, d.lastAddress) + if err := d.syscallIoctl(I2C_SMBUS, unsafe.Pointer(&smbus), 0, sender); err != nil { return err } @@ -362,19 +370,19 @@ func (d *i2cDevice) setAddress(address int) error { } return nil } - // for go vet false positives, see: https://github.com/golang/go/issues/41205 - if err := d.syscallIoctl(I2C_SLAVE, unsafe.Pointer(uintptr(byte(address))), "Setting address"); err != nil { + + if err := d.syscallIoctl(I2C_SLAVE, nil, address, "Setting address"); err != nil { return err } d.lastAddress = address return nil } -func (d *i2cDevice) syscallIoctl(signal uintptr, payload unsafe.Pointer, sender string) (err error) { +func (d *i2cDevice) syscallIoctl(signal uintptr, payload unsafe.Pointer, address int, sender string) (err error) { if err := d.openFileLazy(sender); err != nil { return err } - if _, _, errno := d.sys.syscall(Syscall_SYS_IOCTL, d.file, signal, payload); errno != 0 { + if _, _, errno := d.sys.syscall(Syscall_SYS_IOCTL, d.file, signal, payload, uint16(address)); errno != 0 { return fmt.Errorf("%s failed with syscall.Errno %v", sender, errno) } diff --git a/system/i2c_device_test.go b/system/i2c_device_test.go index 061bf395f..31dc1d333 100644 --- a/system/i2c_device_test.go +++ b/system/i2c_device_test.go @@ -11,18 +11,20 @@ import ( const dev = "/dev/i2c-1" -func getSyscallFuncImpl(errorMask byte) func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) { +func getSyscallFuncImpl( + errorMask byte, +) func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) { // bit 0: error on function query // bit 1: error on set address // bit 2: error on command - return func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) { + return func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) { // function query if (trap == Syscall_SYS_IOCTL) && (a2 == I2C_FUNCS) { if errorMask&0x01 == 0x01 { return 0, 0, 1 } - var funcPtr *uint64 = (*uint64)(unsafe.Pointer(a3)) + var funcPtr *uint64 = (*uint64)(a3) *funcPtr = I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_READ_BYTE_DATA | I2C_FUNC_SMBUS_READ_WORD_DATA | I2C_FUNC_SMBUS_WRITE_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA | @@ -115,7 +117,7 @@ func TestWriteRead(t *testing.T) { func TestReadByte(t *testing.T) { tests := map[string]struct { funcs uint64 - syscallImpl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + syscallImpl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) wantErr string }{ "read_byte_ok": { @@ -124,7 +126,8 @@ func TestReadByte(t *testing.T) { "error_syscall": { funcs: I2C_FUNC_SMBUS_READ_BYTE, syscallImpl: getSyscallFuncImpl(0x04), - wantErr: "SMBus access r/w: 1, command: 0, protocol: 1, address: 2 failed with syscall.Errno operation not permitted", + wantErr: "SMBus access r/w: 1, command: 0, protocol: 1, address: 2 " + + "failed with syscall.Errno operation not permitted", }, "error_not_supported": { wantErr: "SMBus read byte not supported", @@ -159,7 +162,7 @@ func TestReadByte(t *testing.T) { func TestReadByteData(t *testing.T) { tests := map[string]struct { funcs uint64 - syscallImpl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + syscallImpl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) wantErr string }{ "read_byte_data_ok": { @@ -168,7 +171,8 @@ func TestReadByteData(t *testing.T) { "error_syscall": { funcs: I2C_FUNC_SMBUS_READ_BYTE_DATA, syscallImpl: getSyscallFuncImpl(0x04), - wantErr: "SMBus access r/w: 1, command: 1, protocol: 2, address: 3 failed with syscall.Errno operation not permitted", + wantErr: "SMBus access r/w: 1, command: 1, protocol: 2, address: 3 " + + "failed with syscall.Errno operation not permitted", }, "error_not_supported": { wantErr: "SMBus read byte data not supported", @@ -206,7 +210,7 @@ func TestReadByteData(t *testing.T) { func TestReadWordData(t *testing.T) { tests := map[string]struct { funcs uint64 - syscallImpl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + syscallImpl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) wantErr string }{ "read_word_data_ok": { @@ -215,7 +219,8 @@ func TestReadWordData(t *testing.T) { "error_syscall": { funcs: I2C_FUNC_SMBUS_READ_WORD_DATA, syscallImpl: getSyscallFuncImpl(0x04), - wantErr: "SMBus access r/w: 1, command: 2, protocol: 3, address: 4 failed with syscall.Errno operation not permitted", + wantErr: "SMBus access r/w: 1, command: 2, protocol: 3, address: 4 " + + "failed with syscall.Errno operation not permitted", }, "error_not_supported": { wantErr: "SMBus read word data not supported", @@ -270,7 +275,7 @@ func TestReadBlockData(t *testing.T) { ) tests := map[string]struct { funcs uint64 - syscallImpl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + syscallImpl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) wantErr string }{ "read_block_data_ok": { @@ -279,7 +284,8 @@ func TestReadBlockData(t *testing.T) { "error_syscall": { funcs: I2C_FUNC_SMBUS_READ_I2C_BLOCK, syscallImpl: getSyscallFuncImpl(0x04), - wantErr: "SMBus access r/w: 1, command: 3, protocol: 8, address: 5 failed with syscall.Errno operation not permitted", + wantErr: "SMBus access r/w: 1, command: 3, protocol: 8, address: 5 " + + "failed with syscall.Errno operation not permitted", }, "error_from_used_fallback_if_not_supported": { wantErr: "Read 1 bytes from device by sysfs, expected 10", @@ -315,7 +321,7 @@ func TestReadBlockData(t *testing.T) { func TestWriteByte(t *testing.T) { tests := map[string]struct { funcs uint64 - syscallImpl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + syscallImpl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) wantErr string }{ "write_byte_ok": { @@ -324,7 +330,8 @@ func TestWriteByte(t *testing.T) { "error_syscall": { funcs: I2C_FUNC_SMBUS_WRITE_BYTE, syscallImpl: getSyscallFuncImpl(0x04), - wantErr: "SMBus access r/w: 0, command: 68, protocol: 1, address: 6 failed with syscall.Errno operation not permitted", + wantErr: "SMBus access r/w: 0, command: 68, protocol: 1, address: 6 " + + "failed with syscall.Errno operation not permitted", }, "error_not_supported": { wantErr: "SMBus write byte not supported", @@ -357,7 +364,7 @@ func TestWriteByte(t *testing.T) { func TestWriteByteData(t *testing.T) { tests := map[string]struct { funcs uint64 - syscallImpl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + syscallImpl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) wantErr string }{ "write_byte_data_ok": { @@ -366,7 +373,8 @@ func TestWriteByteData(t *testing.T) { "error_syscall": { funcs: I2C_FUNC_SMBUS_WRITE_BYTE_DATA, syscallImpl: getSyscallFuncImpl(0x04), - wantErr: "SMBus access r/w: 0, command: 4, protocol: 2, address: 7 failed with syscall.Errno operation not permitted", + wantErr: "SMBus access r/w: 0, command: 4, protocol: 2, address: 7 " + + "failed with syscall.Errno operation not permitted", }, "error_not_supported": { wantErr: "SMBus write byte data not supported", @@ -404,7 +412,7 @@ func TestWriteByteData(t *testing.T) { func TestWriteWordData(t *testing.T) { tests := map[string]struct { funcs uint64 - syscallImpl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + syscallImpl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) wantErr string }{ "write_word_data_ok": { @@ -413,7 +421,8 @@ func TestWriteWordData(t *testing.T) { "error_syscall": { funcs: I2C_FUNC_SMBUS_WRITE_WORD_DATA, syscallImpl: getSyscallFuncImpl(0x04), - wantErr: "SMBus access r/w: 0, command: 5, protocol: 3, address: 8 failed with syscall.Errno operation not permitted", + wantErr: "SMBus access r/w: 0, command: 5, protocol: 3, address: 8 " + + "failed with syscall.Errno operation not permitted", }, "error_not_supported": { wantErr: "SMBus write word data not supported", @@ -469,7 +478,7 @@ func TestWriteBlockData(t *testing.T) { ) tests := map[string]struct { funcs uint64 - syscallImpl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + syscallImpl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) wantErr string }{ "write_block_data_ok": { @@ -478,7 +487,8 @@ func TestWriteBlockData(t *testing.T) { "error_syscall": { funcs: I2C_FUNC_SMBUS_WRITE_I2C_BLOCK, syscallImpl: getSyscallFuncImpl(0x04), - wantErr: "SMBus access r/w: 0, command: 6, protocol: 8, address: 9 failed with syscall.Errno operation not permitted", + wantErr: "SMBus access r/w: 0, command: 6, protocol: 8, address: 9 " + + "failed with syscall.Errno operation not permitted", }, } for name, tc := range tests { @@ -531,7 +541,7 @@ func Test_queryFunctionality(t *testing.T) { tests := map[string]struct { requested uint64 dev string - syscallImpl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + syscallImpl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) wantErr string wantFile bool wantFuncs uint64 diff --git a/system/syscall.go b/system/syscall.go index 633bd9ba9..b90b1e976 100644 --- a/system/syscall.go +++ b/system/syscall.go @@ -21,8 +21,30 @@ const ( type nativeSyscall struct{} // Syscall calls the native unix.Syscall, implements the SystemCaller interface -func (sys *nativeSyscall) syscall(trap uintptr, f File, signal uintptr, payload unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) { - r1, r2, errNo := unix.Syscall(trap, f.Fd(), signal, uintptr(payload)) +// Note: It would be possible to transfer the address as an unsafe.Pointer to e.g. a byte, uint16 or integer variable. +// The unpack process here would be as follows: +// * convert the payload back to the pointer: addrPtr := (*byte)(payload) +// * call with the content converted to uintptr: r1, r2, errNo = unix.Syscall(trap, f.Fd(), signal, uintptr(*addrPtr)) +// This has the main disadvantage, that if someone change the type of the address at caller side, the compiler will not +// detect this problem and this unpack procedure would cause unpredictable results. +// So the decision was taken to give the address here as a separate parameter, although it is not used in every call. +// Note also, that the size of the address variable at Kernel side is u16, therefore uint16 is used here. +func (sys *nativeSyscall) syscall( + trap uintptr, + f File, + signal uintptr, + payload unsafe.Pointer, + address uint16, +) (r1, r2 uintptr, err SyscallErrno) { + var errNo unix.Errno + if signal == I2C_SLAVE { + // this is the setup for the address, it just needs to be converted to an uintptr, + // the given payload is not used in this case, see the comment on the function + r1, r2, errNo = unix.Syscall(trap, f.Fd(), signal, uintptr(address)) + } else { + r1, r2, errNo = unix.Syscall(trap, f.Fd(), signal, uintptr(payload)) + } + return r1, r2, SyscallErrno(errNo) } diff --git a/system/syscall_mock.go b/system/syscall_mock.go index 45317c97d..1be3ffeba 100644 --- a/system/syscall_mock.go +++ b/system/syscall_mock.go @@ -13,18 +13,25 @@ type mockSyscall struct { smbus *i2cSmbusIoctlData sliceSize uint8 dataSlice []byte - Impl func(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err SyscallErrno) + Impl func(trap, a1, a2 uintptr, a3 unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) } // Syscall calls the user defined implementation, used for tests, implements the SystemCaller interface -func (sys *mockSyscall) syscall(trap uintptr, f File, signal uintptr, payload unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) { +func (sys *mockSyscall) syscall( + trap uintptr, + f File, + signal uintptr, + payload unsafe.Pointer, + address uint16, +) (r1, r2 uintptr, err SyscallErrno) { sys.lastTrap = trap // points to the used syscall (e.g. "SYS_IOCTL") sys.lastFile = f // a character device file (e.g. file to path "/dev/i2c-1") sys.lastSignal = signal // points to used function type (e.g. I2C_SMBUS, I2C_RDWR) if signal == I2C_SLAVE { - // in this case the uintptr corresponds the address - sys.devAddress = uintptr(payload) + // this is the setup for the address, it needs to be converted to an uintptr, + // the given payload is not used in this case, see the comment on the function used for production + sys.devAddress = uintptr(address) } if signal == I2C_SMBUS { @@ -54,7 +61,7 @@ func (sys *mockSyscall) syscall(trap uintptr, f File, signal uintptr, payload un // call mock implementation if sys.Impl != nil { - return sys.Impl(trap, f.Fd(), signal, uintptr(payload)) + return sys.Impl(trap, f.Fd(), signal, payload) } return 0, 0, 0 } diff --git a/system/system.go b/system/system.go index 708885265..30ae3f39e 100644 --- a/system/system.go +++ b/system/system.go @@ -31,8 +31,15 @@ type filesystem interface { // systemCaller represents unexposed Syscall interface to allow the switch between native and mocked implementation // Prevent unsafe call, since go 1.15, see "Pattern 4" in: https://go101.org/article/unsafe.html +// For go vet false positives, see: https://github.com/golang/go/issues/41205 type systemCaller interface { - syscall(trap uintptr, f File, signal uintptr, payload unsafe.Pointer) (r1, r2 uintptr, err SyscallErrno) + syscall( + trap uintptr, + f File, + signal uintptr, + payload unsafe.Pointer, + address uint16, + ) (r1, r2 uintptr, err SyscallErrno) } // digitalPinAccesser represents unexposed interface to allow the switch between different implementations and