From feef0206ad6babf52260aef0eb9af4492ca59ce7 Mon Sep 17 00:00:00 2001 From: Ping Qiu Date: Sat, 28 Feb 2026 10:12:16 -0800 Subject: [PATCH] fix: address code review findings (nil handler, CmdSN, Data-Out order) 1. Discovery session nil handler crash: reject SCSI commands with Reject PDU when s.scsi is nil (discovery sessions have no target). 2. CmdSN window enforcement: validate incoming CmdSN against [ExpCmdSN, MaxCmdSN] using serial arithmetic. Drop out-of-window commands per RFC 7143 section 4.2.2.1. 3. Data-Out buffer offset validation: enforce BufferOffset == received for ordered data (DataPDUInOrder=Yes). Prevents silent corruption from out-of-order or overlapping data. 4. ImmediateData enforcement: reject immediate data in SCSI command PDU when negotiated ImmediateData=No. 5. UNMAP descriptor length alignment: reject blockDescLen not a multiple of 16 bytes. Co-Authored-By: Claude Opus 4.6 --- weed/storage/blockvol/iscsi/dataio.go | 12 ++++++-- weed/storage/blockvol/iscsi/scsi.go | 3 ++ weed/storage/blockvol/iscsi/session.go | 42 ++++++++++++++++++++++++-- 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/weed/storage/blockvol/iscsi/dataio.go b/weed/storage/blockvol/iscsi/dataio.go index 223a97085..17f2be678 100644 --- a/weed/storage/blockvol/iscsi/dataio.go +++ b/weed/storage/blockvol/iscsi/dataio.go @@ -6,9 +6,10 @@ import ( ) var ( - ErrDataSNOrder = errors.New("iscsi: Data-Out DataSN out of order") - ErrDataOverflow = errors.New("iscsi: data exceeds expected transfer length") - ErrDataIncomplete = errors.New("iscsi: data transfer incomplete") + ErrDataSNOrder = errors.New("iscsi: Data-Out DataSN out of order") + ErrDataOffsetOrder = errors.New("iscsi: Data-Out buffer offset out of order") + ErrDataOverflow = errors.New("iscsi: data exceeds expected transfer length") + ErrDataIncomplete = errors.New("iscsi: data transfer incomplete") ) // DataInWriter splits a read response into multiple Data-In PDUs, respecting @@ -140,6 +141,11 @@ func (c *DataOutCollector) AddDataOut(pdu *PDU) error { return ErrDataOverflow } + // Validate buffer offset matches received position (DataPDUInOrder/DataSequenceInOrder) + if offset != c.received { + return ErrDataOffsetOrder + } + copy(c.buf[offset:], data) c.received += uint32(len(data)) diff --git a/weed/storage/blockvol/iscsi/scsi.go b/weed/storage/blockvol/iscsi/scsi.go index ef3f87a7c..6f33e71fc 100644 --- a/weed/storage/blockvol/iscsi/scsi.go +++ b/weed/storage/blockvol/iscsi/scsi.go @@ -405,6 +405,9 @@ func (h *SCSIHandler) unmap(cdb [16]byte, dataOut []byte) SCSIResult { if int(blockDescLen)+8 > len(dataOut) { return illegalRequest(ASCInvalidFieldInCDB, ASCQLuk) } + if blockDescLen%16 != 0 { + return illegalRequest(ASCInvalidFieldInCDB, ASCQLuk) + } // Each UNMAP block descriptor is 16 bytes descData := dataOut[8 : 8+blockDescLen] diff --git a/weed/storage/blockvol/iscsi/session.go b/weed/storage/blockvol/iscsi/session.go index 2429d33fb..de2b1f39b 100644 --- a/weed/storage/blockvol/iscsi/session.go +++ b/weed/storage/blockvol/iscsi/session.go @@ -45,6 +45,10 @@ type Session struct { negotiator *LoginNegotiator loginDone bool + // Negotiated session parameters + negImmediateData bool + negInitialR2T bool + // Data sequencing dataInWriter *DataInWriter @@ -177,8 +181,12 @@ func (s *Session) handleLogin(pdu *PDU) error { s.state = SessionLoggedIn result := s.negotiator.Result() s.dataInWriter = NewDataInWriter(uint32(result.MaxRecvDataSegLen)) + s.negImmediateData = result.ImmediateData + s.negInitialR2T = result.InitialR2T - // Bind SCSI handler to the device for the target the initiator logged into + // Bind SCSI handler to the device for the target the initiator logged into. + // Discovery sessions have no TargetName — s.scsi stays nil, which is + // checked in handleSCSICmd. if s.devices != nil && result.TargetName != "" { dev := s.devices.LookupDevice(result.TargetName) s.scsi = NewSCSIHandler(dev) @@ -215,12 +223,25 @@ func (s *Session) handleSCSICmd(pdu *PDU) error { return s.sendReject(pdu, 0x0b) // protocol error } + // Discovery sessions have no SCSI handler — reject commands + if s.scsi == nil { + return s.sendReject(pdu, 0x04) // command not supported + } + cdb := pdu.CDB() itt := pdu.InitiatorTaskTag() flags := pdu.OpSpecific1() - // Advance CmdSN + // CmdSN validation for non-immediate commands (RFC 7143 section 4.2.2.1) if !pdu.Immediate() { + cmdSN := pdu.CmdSN() + expCmdSN := s.expCmdSN.Load() + maxCmdSN := s.maxCmdSN.Load() + // CmdSN is within window if ExpCmdSN <= CmdSN <= MaxCmdSN (serial arithmetic) + if !cmdSNInWindow(cmdSN, expCmdSN, maxCmdSN) { + s.logger.Printf("CmdSN %d out of window [%d, %d], dropping", cmdSN, expCmdSN, maxCmdSN) + return nil // silently drop per RFC 7143 + } s.advanceCmdSN() } @@ -233,8 +254,12 @@ func (s *Session) handleSCSICmd(pdu *PDU) error { if isWrite && expectedLen > 0 { collector := NewDataOutCollector(expectedLen) - // Immediate data + // Immediate data — enforce negotiated ImmediateData flag if len(pdu.DataSegment) > 0 { + if !s.negImmediateData { + // ImmediateData=No but initiator sent data — reject + return s.sendCheckCondition(itt, SenseIllegalRequest, ASCInvalidFieldInCDB, ASCQLuk) + } if err := collector.AddImmediateData(pdu.DataSegment); err != nil { return s.sendCheckCondition(itt, SenseIllegalRequest, ASCInvalidFieldInCDB, ASCQLuk) } @@ -388,6 +413,17 @@ func (s *Session) advanceCmdSN() { s.maxCmdSN.Add(1) } +// cmdSNInWindow checks if cmdSN is within [expCmdSN, maxCmdSN] using +// serial number arithmetic (RFC 7143 section 4.2.2.1). Handles uint32 wrap. +func cmdSNInWindow(cmdSN, expCmdSN, maxCmdSN uint32) bool { + // Serial comparison: a <= b means (b - a) < 2^31 + return serialLE(expCmdSN, cmdSN) && serialLE(cmdSN, maxCmdSN) +} + +func serialLE(a, b uint32) bool { + return a == b || int32(b-a) > 0 +} + func (s *Session) sendReject(origPDU *PDU, reason uint8) error { resp := &PDU{} resp.SetOpcode(OpReject)