[op] Make PacketBuffer Clone() do a deeper copy.

Earlier PacketBuffer.Clone() would do a shallow top level copy of the packet
buffer - which involved sharing the *buffer.Buffer between packets. Reading
or writing to the buffer in one packet would impact the other.

This caused modifications in one packet to affect the other's pkt.Views() which
is not desired. Change the clone to do a deeper copy of the underlying buffer
list and buffer pointers. The payload buffers (which are immutable) are still
shared. This change makes the Clone() operation more expensive as we now need to
allocate the entire buffer list.

Added unit test to test integrity of packet data after cloning.

Reported-by: syzbot+7ffff9a82a227b8f2e31@syzkaller.appspotmail.com
Reported-by: syzbot+7d241de0d9072b2b6075@syzkaller.appspotmail.com
Reported-by: syzbot+212bc4d75802fa461521@syzkaller.appspotmail.com
PiperOrigin-RevId: 390277713

3 files changed, 46 insertions, 8 deletions

diff --git a/pkg/buffer/view.go b/pkg/buffer/view.go
index 7bcfcd543..a4610f977 100644
--- a/pkg/buffer/view.go
+++ b/pkg/buffer/view.go
@@ -378,6 +378,20 @@ func (v *View) Copy() (other View) {
 	return
 }
 
+// Clone makes a more shallow copy compared to Copy. The underlying payload
+// slice (buffer.data) is shared but the buffers themselves are copied.
+func (v *View) Clone() *View {
+	other := &View{
+		size: v.size,
+	}
+	for buf := v.data.Front(); buf != nil; buf = buf.Next() {
+		newBuf := other.pool.getNoInit()
+		*newBuf = *buf
+		other.data.PushBack(newBuf)
+	}
+	return other
+}
+
 // Apply applies the given function across all valid data.
 func (v *View) Apply(fn func([]byte)) {
 	for buf := v.data.Front(); buf != nil; buf = buf.Next() {
diff --git a/pkg/tcpip/stack/packet_buffer.go b/pkg/tcpip/stack/packet_buffer.go
index 9192d8433..29c22bfd4 100644
--- a/pkg/tcpip/stack/packet_buffer.go
+++ b/pkg/tcpip/stack/packet_buffer.go
@@ -282,14 +282,12 @@ func (pk *PacketBuffer) headerView(typ headerType) tcpipbuffer.View {
 	return v
 }
 
-// Clone makes a shallow copy of pk.
-//
-// Clone should be called in such cases so that no modifications is done to
-// underlying packet payload.
+// Clone makes a semi-deep copy of pk. The underlying packet payload is
+// shared. Hence, no modifications is done to underlying packet payload.
 func (pk *PacketBuffer) Clone() *PacketBuffer {
 	return &PacketBuffer{
 		PacketBufferEntry:            pk.PacketBufferEntry,
-		buf:                          pk.buf,
+		buf:                          pk.buf.Clone(),
 		reserved:                     pk.reserved,
 		pushed:                       pk.pushed,
 		consumed:                     pk.consumed,
@@ -321,14 +319,14 @@ func (pk *PacketBuffer) Network() header.Network {
 	}
 }
 
-// CloneToInbound makes a shallow copy of the packet buffer to be used as an
-// inbound packet.
+// CloneToInbound makes a semi-deep copy of the packet buffer (similar to
+// Clone) to be used as an inbound packet.
 //
 // See PacketBuffer.Data for details about how a packet buffer holds an inbound
 // packet.
 func (pk *PacketBuffer) CloneToInbound() *PacketBuffer {
 	newPk := &PacketBuffer{
-		buf: pk.buf,
+		buf: pk.buf.Clone(),
 		// Treat unfilled header portion as reserved.
 		reserved: pk.AvailableHeaderBytes(),
 	}
diff --git a/pkg/tcpip/stack/packet_buffer_test.go b/pkg/tcpip/stack/packet_buffer_test.go
index a8da34992..87b023445 100644
--- a/pkg/tcpip/stack/packet_buffer_test.go
+++ b/pkg/tcpip/stack/packet_buffer_test.go
@@ -123,6 +123,32 @@ func TestPacketHeaderPush(t *testing.T) {
 	}
 }
 
+func TestPacketBufferClone(t *testing.T) {
+	data := concatViews(makeView(20), makeView(30), makeView(40))
+	pk := NewPacketBuffer(PacketBufferOptions{
+		// Make a copy of data to make sure our truth data won't be taint by
+		// PacketBuffer.
+		Data: buffer.NewViewFromBytes(data).ToVectorisedView(),
+	})
+
+	bytesToDelete := 30
+	originalSize := data.Size()
+
+	clonedPks := []*PacketBuffer{
+		pk.Clone(),
+		pk.CloneToInbound(),
+	}
+	pk.Data().DeleteFront(bytesToDelete)
+	if got, want := pk.Data().Size(), originalSize-bytesToDelete; got != want {
+		t.Errorf("original packet was not changed: size expected = %d, got = %d", want, got)
+	}
+	for _, clonedPk := range clonedPks {
+		if got := clonedPk.Data().Size(); got != originalSize {
+			t.Errorf("cloned packet should not be modified: expected size = %d, got = %d", originalSize, got)
+		}
+	}
+}
+
 func TestPacketHeaderConsume(t *testing.T) {
 	for _, test := range []struct {
 		name      string