From e0ad90bcd93a4f61ab4fa085c1c5ca929355a684 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=96=E7=95=8C?= Date: Sat, 16 May 2026 15:24:10 +0800 Subject: [PATCH] usbip: validate ret_submit iso descriptors and lock handoff Close fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2: Darwin IN iso responses accepted malformed RET_SUBMIT descriptors — aggregate ActualLength was checked but per-descriptor Offset/Length were not, and ScatterIsoResponse silently clamped bad values into apparent success. ValidateIsoResponse now lives in iso_scheduler.go beside EncodeIsoSubmit/RebaseFrame and enforces the single-packet shape (Offset == 0, Length == requestLen, ActualLength <= Length, sum == header, payload covers range). pendingTransfer.validateResponse routes every RET_SUBMIT shape check through one named seam so future defects land in one place, and ScatterIsoResponse drops its defensive clamps so the data-movement primitive can no longer mask a validation gap. P2: kernelHandoffSession.Close mutated h.conn/h.monitorFile/h.relayConn outside stateAccess while Start reads them under that lock; the server registers prepared sessions before Start, so Close-before-Start can race the direct-TCP path during import-time shutdown. Close now snapshots and nils the three fields under stateAccess before closing the locals via closeOnce, restoring symmetry with Start's under-lock reads and matching the copy-under-lock, close-outside idiom established by exportLedger.CloseAllSubscribers and ServerService.Close. --- service/usbip/endpoint_darwin.go | 51 +++++++++++++++++++--------- service/usbip/handoff_linux.go | 16 +++++---- service/usbip/iso_scheduler.go | 58 +++++++++++++++++++++++--------- 3 files changed, 88 insertions(+), 37 deletions(-) diff --git a/service/usbip/endpoint_darwin.go b/service/usbip/endpoint_darwin.go index 374e6517d..4d20c2fe9 100644 --- a/service/usbip/endpoint_darwin.go +++ b/service/usbip/endpoint_darwin.go @@ -207,33 +207,52 @@ func (e *darwinEndpoint) finalizePending(pending *pendingTransfer) { } } -// accept validates a RET_SUBMIT against the original request and, for IN -// transfers, scatters the payload into the Apple-owned buffer. The returned -// length is guaranteed to be in [0, p.requestLen] regardless of direction. -// A non-nil err signals a protocol violation; the caller is expected to -// cancel the endpoint so no further wire-corrupt completions are delivered -// to IOUSBHost. -func (p *pendingTransfer) accept(response SubmitResponse) (int32, int, error) { +// validateResponse reconciles a RET_SUBMIT against the original request shape. +// Every wire-shape rule lives here so accept can assume well-formed input and +// future defects land in one place. Returns the wire-level errno status and a +// non-nil err on protocol violation. +func (p *pendingTransfer) validateResponse(response SubmitResponse) (int32, error) { if response.ActualLength < 0 { - return -int32(unix.EPROTO), 0, errResponseNegativeActualLength + return -int32(unix.EPROTO), errResponseNegativeActualLength + } + if int(response.ActualLength) > p.requestLen { + return -int32(unix.EOVERFLOW), errResponseOverflow + } + if p.direction != USBIPDirIn { + return 0, nil + } + if len(response.Buffer) > p.requestLen { + return -int32(unix.EOVERFLOW), errResponseOverflow + } + if len(response.IsoPackets) > 0 { + err := ValidateIsoResponse(p.requestLen, int(response.ActualLength), response.IsoPackets, len(response.Buffer)) + if err != nil { + return -int32(unix.EPROTO), err + } + } + return 0, nil +} + +// accept validates a RET_SUBMIT against the original request and, for IN +// transfers, scatters the payload into the Apple-owned buffer. A non-nil err +// signals a protocol violation; the caller is expected to cancel the endpoint +// so no further wire-corrupt completions are delivered to IOUSBHost. +func (p *pendingTransfer) accept(response SubmitResponse) (int32, int, error) { + errStatus, err := p.validateResponse(response) + if err != nil { + return errStatus, 0, err } actualLength := int(response.ActualLength) - if actualLength > p.requestLen { - return -int32(unix.EOVERFLOW), 0, errResponseOverflow - } if p.direction != USBIPDirIn { return response.Status, actualLength, nil } - if len(response.Buffer) > p.requestLen { - return -int32(unix.EOVERFLOW), 0, errResponseOverflow - } copyLength := min(actualLength, len(response.Buffer)) if copyLength > 0 && p.bufferPtr != nil { + dst := unsafe.Slice((*byte)(p.bufferPtr), p.requestLen) if len(response.IsoPackets) > 0 { - dst := unsafe.Slice((*byte)(p.bufferPtr), p.requestLen) ScatterIsoResponse(dst, response.Buffer[:copyLength], response.IsoPackets) } else { - copy(unsafe.Slice((*byte)(p.bufferPtr), copyLength), response.Buffer[:copyLength]) + copy(dst[:copyLength], response.Buffer[:copyLength]) } } return response.Status, actualLength, nil diff --git a/service/usbip/handoff_linux.go b/service/usbip/handoff_linux.go index bd8fbe186..09a052aa9 100644 --- a/service/usbip/handoff_linux.go +++ b/service/usbip/handoff_linux.go @@ -108,17 +108,21 @@ func (h *kernelHandoffSession) Err() error { func (h *kernelHandoffSession) Close() error { h.stateAccess.Lock() h.closed = true + conn := h.conn + monitorFile := h.monitorFile + relayConn := h.relayConn + h.conn = nil + h.monitorFile = nil + h.relayConn = nil h.stateAccess.Unlock() + h.closeOnce.Do(func() { h.closeErr = E.Errors( h.closeKernelFD(), - common.Close(h.monitorFile), - common.Close(h.relayConn), - common.Close(h.conn), + common.Close(monitorFile), + common.Close(relayConn), + common.Close(conn), ) - h.monitorFile = nil - h.relayConn = nil - h.conn = nil }) h.markDone(nil) return h.closeErr diff --git a/service/usbip/iso_scheduler.go b/service/usbip/iso_scheduler.go index 5995c3bbd..b099867a0 100644 --- a/service/usbip/iso_scheduler.go +++ b/service/usbip/iso_scheduler.go @@ -2,6 +2,10 @@ package usbip +import ( + E "github.com/sagernet/sing/common/exceptions" +) + // EncodeIsoSubmit fills the isochronous SUBMIT fields on base. When asap is // true, the wire-level ASAP flag is set and StartFrame is zeroed. Otherwise // RebaseFrame recovers the absolute frame number from the controller's @@ -31,26 +35,50 @@ func RebaseFrame(currentFrame uint64, low8 uint8) uint64 { return base } +var ( + errIsoDescriptorCount = E.New("RET_SUBMIT iso descriptor count mismatch") + errIsoDescriptorRange = E.New("RET_SUBMIT iso descriptor range mismatch") + errIsoDescriptorActualLength = E.New("RET_SUBMIT iso descriptor actual_length exceeds length") + errIsoDescriptorSum = E.New("RET_SUBMIT iso descriptor actual_length sum does not match header") + errIsoPayloadShort = E.New("RET_SUBMIT iso payload shorter than descriptor range") +) + +// ValidateIsoResponse enforces the per-descriptor invariants of a RET_SUBMIT +// against the original CMD_SUBMIT shape. startIsoTransfer emits single-packet +// CMD_SUBMITs that cover the whole request, so the response must mirror that +// exact shape. If multi-packet ISO submits are added later, the count check +// relaxes to a non-empty count and the offset/length check becomes a per- +// descriptor walk that proves coverage is non-overlapping and in-range. +func ValidateIsoResponse(requestLen int, actualLength int, packets []IsoPacketDescriptor, payloadLen int) error { + if len(packets) != 1 { + return E.Extend(errIsoDescriptorCount, "expected 1, got ", len(packets)) + } + descriptor := packets[0] + if descriptor.Offset != 0 || int(descriptor.Length) != requestLen { + return E.Extend(errIsoDescriptorRange, "offset ", descriptor.Offset, ", length ", descriptor.Length, ", request ", requestLen) + } + if descriptor.ActualLength < 0 || descriptor.ActualLength > descriptor.Length { + return E.Extend(errIsoDescriptorActualLength, "actual_length ", descriptor.ActualLength, ", length ", descriptor.Length) + } + if int(descriptor.ActualLength) != actualLength { + return E.Extend(errIsoDescriptorSum, "sum ", descriptor.ActualLength, " != header ", actualLength) + } + if int(descriptor.ActualLength) > payloadLen { + return E.Extend(errIsoPayloadShort, "actual_length ", descriptor.ActualLength, " > payload ", payloadLen) + } + return nil +} + +// ScatterIsoResponse copies frame data from payload into dst at the offsets +// declared by packets. Caller MUST have called ValidateIsoResponse against the +// same descriptors and payload before invoking; this function trusts every +// offset and length and will panic on slice bounds for malformed input. func ScatterIsoResponse(dst, payload []byte, packets []IsoPacketDescriptor) { cursor := 0 for i := range packets { length := int(packets[i].ActualLength) - if length <= 0 { - continue - } - if cursor+length > len(payload) { - length = len(payload) - cursor - if length <= 0 { - return - } - } offset := int(packets[i].Offset) - if offset < 0 || offset >= len(dst) { - cursor += length - continue - } - end := min(offset+length, len(dst)) - copy(dst[offset:end], payload[cursor:cursor+(end-offset)]) + copy(dst[offset:offset+length], payload[cursor:cursor+length]) cursor += length } }