Add locking around ringbuffer fields and reserve in packetmmap endpoint.

Reported-by: syzbot+740f3fdf0245dde5e8cc@syzkaller.appspotmail.com
Reported-by: syzbot+f22c4f574091579bcc79@syzkaller.appspotmail.com
PiperOrigin-RevId: 759230496

Author: manninglucas

pkg/sentry/socket/netstack/packetmmap/endpoint.go

@@ -188,7 +188,11 @@ func (m *Endpoint) Readiness(mask waiter.EventMask) waiter.EventMask {
 }
 
 // HandlePacket implements stack.PacketMMapEndpoint.HandlePacket.
-func (m *Endpoint) HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtocolNumber, pkt *stack.PacketBuffer) {
+func (m *Endpoint) HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtocolNumber, pkt *stack.PacketBuffer) bool {
+	if !m.Mapped() {
+		return false
+	}
+
 	const minMacLen = 16
 	var (
 		status                           = uint32(linux.TP_STATUS_USER)
@@ -200,11 +204,12 @@ func (m *Endpoint) HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtoco
 	cooked := m.cooked
 	stats := m.stack.Stats()
 	hdrLen := m.headerLen
+	reserve := m.reserve
 	if !m.rxRingBuffer.hasRoom() {
 		m.mu.Unlock()
 		stats.DroppedPackets.Increment()
 		m.dropped.Add(1)
-		return
+		return true
 	}
 	m.mu.Unlock()
 
@@ -220,14 +225,14 @@ func (m *Endpoint) HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtoco
 		pktBuf.TrimFront(int64(len(pkt.LinkHeader().Slice()) + len(pkt.VirtioNetHeader().Slice())))
 		// Cooked packet endpoints don't include the link-headers in received
 		// packets.
-		netOffset = linux.TPacketAlign(hdrLen+minMacLen) + m.reserve
+		netOffset = linux.TPacketAlign(hdrLen+minMacLen) + reserve
 		macOffset = netOffset
 	} else {
 		virtioNetHdrLen := uint32(len(pkt.VirtioNetHeader().Slice()))
 		macLen := uint32(len(pkt.LinkHeader().Slice())) + virtioNetHdrLen
-		netOffset = linux.TPacketAlign(hdrLen+macLen) + m.reserve
+		netOffset = linux.TPacketAlign(hdrLen+macLen) + reserve
 		if macLen < minMacLen {
-			netOffset = linux.TPacketAlign(hdrLen+minMacLen) + m.reserve
+			netOffset = linux.TPacketAlign(hdrLen+minMacLen) + reserve
 		}
 		if virtioNetHdrLen > 0 {
 			netOffset += virtioNetHdrLen
@@ -237,16 +242,16 @@ func (m *Endpoint) HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtoco
 	if netOffset > uint32(^uint16(0)) {
 		stats.DroppedPackets.Increment()
 		m.dropped.Add(1)
-		return
+		return true
 	}
 	dataLength = uint32(pktBuf.Size())
 
 	// If the packet is too large to fit in the ring buffer, copy it to the
 	// receive queue.
-	if macOffset+dataLength > m.rxRingBuffer.frameSize {
+	if macOffset+dataLength > m.rxRingBuffer.FrameSize() {
 		clone = pkt.Clone()
 		defer clone.DecRef()
-		dataLength = m.rxRingBuffer.frameSize - macOffset
+		dataLength = m.rxRingBuffer.FrameSize() - macOffset
 		if int(dataLength) < 0 {
 			dataLength = 0
 		}
@@ -258,15 +263,15 @@ func (m *Endpoint) HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtoco
 		m.mu.Unlock()
 		stats.DroppedPackets.Increment()
 		m.dropped.Add(1)
-		return
+		return true
 	}
 
 	slot, ok := m.rxRingBuffer.testAndMarkHead()
 	if !ok {
 		m.mu.Unlock()
 		stats.DroppedPackets.Increment()
 		m.dropped.Add(1)
-		return
+		return true
 	}
 	m.rxRingBuffer.incHead()
 
@@ -288,18 +293,19 @@ func (m *Endpoint) HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtoco
 	if err := m.rxRingBuffer.writeFrame(slot, hdrView, pktBuf); err != nil {
 		stats.DroppedPackets.Increment()
 		m.dropped.Add(1)
-		return
+		return true
 	}
 
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	if err := m.rxRingBuffer.writeStatus(slot, status); err != nil {
 		stats.DroppedPackets.Increment()
 		m.dropped.Add(1)
-		return
+		return true
 	}
 	m.received.Add(1)
 	m.wq.Notify(waiter.ReadableEvents)
+	return true
 }
 
 // AddMapping implements memmap.Mappable.AddMapping.

pkg/sentry/socket/netstack/packetmmap/ring_buffer.go

@@ -31,14 +31,22 @@ import (
 	"gvisor.dev/gvisor/pkg/tcpip"
 )
 
+// Lock order for the mutexes in ringBuffer:
+//
+//	mu
+//	  dataMu
+//
 // +stateify savable
 type ringBuffer struct {
+	mu sync.Mutex `state:"nosave"`
+	// +checklocks:mu
 	framesPerBlock uint32
-	frameSize      uint32
-	frameMax       uint32
-	blockSize      uint32
-	numBlocks      uint32
-	version        int
+	// +checklocks:mu
+	frameSize uint32
+	// +checklocks:mu
+	frameMax uint32
+	// +checklocks:mu
+	blockSize uint32
 
 	// The following fields are protected by the owning endpoint's mutex.
 	head       uint32
@@ -59,11 +67,12 @@ type ringBuffer struct {
 //
 // The owning endpoint must be locked when calling this function.
 func (rb *ringBuffer) init(ctx context.Context, req *tcpip.TpacketReq) error {
+	rb.mu.Lock()
+	defer rb.mu.Unlock()
 	rb.blockSize = req.TpBlockSize
 	rb.framesPerBlock = req.TpBlockSize / req.TpFrameSize
 	rb.frameMax = req.TpFrameNr - 1
 	rb.frameSize = req.TpFrameSize
-	rb.numBlocks = req.TpBlockNr
 
 	rb.rxOwnerMap = bitmap.New(req.TpFrameNr)
 	rb.head = 0
@@ -92,7 +101,6 @@ func (rb *ringBuffer) destroy() {
 	rb.dataMu.Lock()
 	rb.mf.DecRef(rb.data)
 	rb.dataMu.Unlock()
-	*rb = ringBuffer{}
 }
 
 // AppendTranslation essentially implements memmap.Mappable.Translate, with the
@@ -125,6 +133,12 @@ func (rb *ringBuffer) AppendTranslation(ctx context.Context, required, optional
 	return ts, nil
 }
 
+func (rb *ringBuffer) FrameSize() uint32 {
+	rb.mu.Lock()
+	defer rb.mu.Unlock()
+	return rb.frameSize
+}
+
 // writeStatus writes the status of a frame to the ring buffer's internal
 // mappings at the provided frame number. It also clears the owner map for the
 // frame number if setting it to TP_STATUS_USER.
@@ -134,6 +148,8 @@ func (rb *ringBuffer) writeStatus(frameNum uint32, status uint32) error {
 	if status&linux.TP_STATUS_USER != 0 {
 		rb.rxOwnerMap.Remove(frameNum)
 	}
+	rb.mu.Lock()
+	defer rb.mu.Unlock()
 	ims, err := rb.internalMappingsForFrame(frameNum, hostarch.Write)
 	if err != nil {
 		return err
@@ -148,6 +164,8 @@ func (rb *ringBuffer) writeStatus(frameNum uint32, status uint32) error {
 // writeFrame writes a frame to the ring buffer's internal mappings at the
 // provided frame number.
 func (rb *ringBuffer) writeFrame(frameNum uint32, hdrView *buffer.View, pkt buffer.Buffer) error {
+	rb.mu.Lock()
+	defer rb.mu.Unlock()
 	ims, err := rb.internalMappingsForFrame(frameNum, hostarch.Write)
 	if err != nil {
 		return err
@@ -168,6 +186,8 @@ func (rb *ringBuffer) writeFrame(frameNum uint32, hdrView *buffer.View, pkt buff
 //
 // The owning endpoint must be locked when calling this method.
 func (rb *ringBuffer) incHead() {
+	rb.mu.Lock()
+	defer rb.mu.Unlock()
 	if rb.head == rb.frameMax {
 		rb.head = 0
 	} else {
@@ -179,6 +199,8 @@ func (rb *ringBuffer) incHead() {
 //
 // The owning endpoint must be locked when calling this method.
 func (rb *ringBuffer) currFrameStatus() (uint32, error) {
+	rb.mu.Lock()
+	defer rb.mu.Unlock()
 	return rb.frameStatus(rb.head)
 }
 
@@ -187,6 +209,8 @@ func (rb *ringBuffer) currFrameStatus() (uint32, error) {
 //
 // The owning endpoint must be locked when calling this method.
 func (rb *ringBuffer) prevFrameStatus() (uint32, error) {
+	rb.mu.Lock()
+	defer rb.mu.Unlock()
 	prev := rb.head - 1
 	if rb.head == 0 {
 		prev = rb.frameMax
@@ -225,6 +249,7 @@ func (rb *ringBuffer) bufferSize() uint64 {
 	return rb.size
 }
 
+// +checklocks:rb.mu
 func (rb *ringBuffer) internalMappingsForFrame(frameNum uint32, at hostarch.AccessType) (safemem.BlockSeq, error) {
 	rb.dataMu.RLock()
 	defer rb.dataMu.RUnlock()
@@ -239,6 +264,7 @@ func (rb *ringBuffer) internalMappingsForFrame(frameNum uint32, at hostarch.Acce
 	return rb.mf.MapInternal(frameFR, at)
 }
 
+// +checklocks:rb.mu
 func (rb *ringBuffer) frameStatus(frameNum uint32) (uint32, error) {
 	ims, err := rb.internalMappingsForFrame(frameNum, hostarch.Read)
 	if err != nil {

pkg/tcpip/stack/registration.go

@@ -217,7 +217,8 @@ type PacketMMapOpts struct {
 // mapped packets over the packet transport protocol (PACKET_MMAP).
 type PacketMMapEndpoint interface {
 	// HandlePacket is called by the stack when new packets arrive that
-	// match the endpoint.
+	// match the endpoint. It returns true if the packet was handled by the
+	// endpoint and false otherwise.
 	//
 	// Implementers should treat packet as immutable and should copy it
 	// before modification.
@@ -226,7 +227,7 @@ type PacketMMapEndpoint interface {
 	// should construct its own ethernet header for applications.
 	//
 	// HandlePacket may modify pkt.
-	HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtocolNumber, pkt *PacketBuffer)
+	HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtocolNumber, pkt *PacketBuffer) bool
 
 	// Close releases any resources associated with the endpoint.
 	Close()

pkg/tcpip/transport/packet/endpoint.go

@@ -492,9 +492,10 @@ func (ep *endpoint) GetSockOptInt(opt tcpip.SockOptInt) (int, tcpip.Error) {
 func (ep *endpoint) HandlePacket(nicID tcpip.NICID, netProto tcpip.NetworkProtocolNumber, pkt *stack.PacketBuffer) {
 	ep.packetMmapMu.RLock()
 	if ep.packetMMapEp != nil {
-		ep.packetMMapEp.HandlePacket(nicID, netProto, pkt)
-		ep.packetMmapMu.RUnlock()
-		return
+		if handled := ep.packetMMapEp.HandlePacket(nicID, netProto, pkt); handled {
+			ep.packetMmapMu.RUnlock()
+			return
+		}
 	}
 	ep.packetMmapMu.RUnlock()