pkg/cover: print a warning and discard coverage if module is invalid

Addresses https://github.com/google/syzkaller/issues/4078

Does not fix canonicalization when modules change over time, but
does unblock fuzzing by avoiding a kernel panic when modules
change.

3 files changed, 129 insertions, 24 deletions

diff --git a/pkg/cover/canonicalizer.go b/pkg/cover/canonicalizer.go
index 758ab5593..c7c385aed 100644
--- a/pkg/cover/canonicalizer.go
+++ b/pkg/cover/canonicalizer.go
@@ -4,6 +4,7 @@
 package cover
 
 import (
+	"fmt"
 	"sort"
 
 	"github.com/google/syzkaller/pkg/host"
@@ -33,10 +34,19 @@ type Convert struct {
 	moduleKeys     []uint32
 }
 
+type convertContext struct {
+	errCount int
+	errPC    uint32
+	convert  *Convert
+}
+
 // Contains the offset and final address of each module.
 type canonicalizerModule struct {
 	offset  int
 	endAddr uint32
+	// Discard coverage from current module.
+	// Set to true if module is not present in canonical.
+	discard bool
 }
 
 func NewCanonicalizer(modules []host.KernelModule, flagSignal bool) *Canonicalizer {
@@ -71,22 +81,29 @@ func (can *Canonicalizer) NewInstance(modules []host.KernelModule) *Canonicalize
 	instToCanonicalMap := make(map[uint32]*canonicalizerModule)
 	canonicalToInstMap := make(map[uint32]*canonicalizerModule)
 	for _, module := range modules {
+		discard := false
+		canonicalAddr := uint32(0)
 		canonicalModule, found := can.modules[module.Name]
 		if !found || canonicalModule.Size != module.Size {
-			log.Fatalf("kernel build has changed; instance module %v differs from canonical", module.Name)
+			log.Errorf("kernel build has changed; instance module %v differs from canonical", module.Name)
+			discard = true
+		}
+		if found {
+			canonicalAddr = uint32(canonicalModule.Addr)
 		}
 
 		instAddr := uint32(module.Addr)
-		canonicalAddr := uint32(canonicalModule.Addr)
 
 		canonicalToInstMap[canonicalAddr] = &canonicalizerModule{
 			offset:  int(instAddr) - int(canonicalAddr),
 			endAddr: uint32(module.Size) + canonicalAddr,
+			discard: discard,
 		}
 
 		instToCanonicalMap[instAddr] = &canonicalizerModule{
 			offset:  int(canonicalAddr) - int(instAddr),
 			endAddr: uint32(module.Size) + instAddr,
+			discard: discard,
 		}
 	}
 
@@ -103,18 +120,18 @@ func (can *Canonicalizer) NewInstance(modules []host.KernelModule) *Canonicalize
 	}
 }
 
-func (ci *CanonicalizerInstance) Canonicalize(cov []uint32, sign signal.Serial) {
+func (ci *CanonicalizerInstance) Canonicalize(cov []uint32, sign signal.Serial) ([]uint32, signal.Serial) {
 	if ci.canonical.moduleKeys == nil {
-		return
+		return cov, sign
 	}
-	ci.canonicalize.convertPCs(cov, sign)
+	return ci.canonicalize.convertPCs(cov, sign)
 }
 
-func (ci *CanonicalizerInstance) Decanonicalize(cov []uint32, sign signal.Serial) {
+func (ci *CanonicalizerInstance) Decanonicalize(cov []uint32, sign signal.Serial) ([]uint32, signal.Serial) {
 	if ci.canonical.moduleKeys == nil {
-		return
+		return cov, sign
 	}
-	ci.decanonicalize.convertPCs(cov, sign)
+	return ci.decanonicalize.convertPCs(cov, sign)
 }
 
 func (ci *CanonicalizerInstance) DecanonicalizeFilter(bitmap map[uint32]uint32) map[uint32]uint32 {
@@ -123,8 +140,16 @@ func (ci *CanonicalizerInstance) DecanonicalizeFilter(bitmap map[uint32]uint32)
 		return bitmap
 	}
 	instBitmap := make(map[uint32]uint32)
+	convCtx := &convertContext{convert: ci.decanonicalize}
 	for pc, val := range bitmap {
-		instBitmap[ci.decanonicalize.convertPC(pc)] = val
+		if newPC, ok := ci.decanonicalize.convertPC(pc); ok {
+			instBitmap[newPC] = val
+		} else {
+			convCtx.discard(pc)
+		}
+	}
+	if msg := convCtx.discarded(); msg != "" {
+		log.Logf(4, "error in bitmap conversion: %v", msg)
 	}
 	return instBitmap
 }
@@ -152,26 +177,66 @@ func findModule(pc uint32, moduleKeys []uint32) (moduleIdx int) {
 	return moduleIdx - 1
 }
 
-func (convert *Convert) convertPCs(cov []uint32, sign signal.Serial) {
+func (convert *Convert) convertPCs(cov []uint32, sign signal.Serial) ([]uint32, signal.Serial) {
 	// Convert coverage.
-	for idx, pc := range cov {
-		cov[idx] = convert.convertPC(pc)
+	var retCov []uint32
+	convCtx := &convertContext{convert: convert}
+	for _, pc := range cov {
+		if newPC, ok := convert.convertPC(pc); ok {
+			retCov = append(retCov, newPC)
+		} else {
+			convCtx.discard(pc)
+		}
+	}
+	if msg := convCtx.discarded(); msg != "" {
+		log.Logf(4, "error in PC conversion: %v", msg)
 	}
 	// Convert signals.
+	retSign := &signal.Serial{}
+	convCtx = &convertContext{convert: convert}
 	for idx, elem := range sign.Elems {
-		sign.UpdateElem(idx, convert.convertPC(uint32(elem)))
+		if newSign, ok := convert.convertPC(uint32(elem)); ok {
+			retSign.AddElem(newSign, sign.Prios[idx])
+		} else {
+			convCtx.discard(uint32(elem))
+		}
 	}
+	if msg := convCtx.discarded(); msg != "" {
+		log.Logf(4, "error in signal conversion: %v", msg)
+	}
+	return retCov, *retSign
 }
 
-func (convert *Convert) convertPC(pc uint32) uint32 {
+func (convert *Convert) convertPC(pc uint32) (uint32, bool) {
 	moduleIdx := findModule(pc, convert.moduleKeys)
 	// Check if address is above the first module offset.
 	if moduleIdx >= 0 {
-		module := convert.conversionHash[convert.moduleKeys[moduleIdx]]
+		module, found := convert.conversionHash[convert.moduleKeys[moduleIdx]]
+		if !found {
+			return pc, false
+		}
 		// If the address is within the found module add the offset.
 		if pc < module.endAddr {
+			if module.discard {
+				return pc, false
+			}
 			pc = uint32(int(pc) + module.offset)
 		}
 	}
-	return pc
+	return pc, true
+}
+
+func (cc *convertContext) discarded() string {
+	if cc.errCount == 0 {
+		return ""
+	}
+	errMsg := fmt.Sprintf("discarded 0x%x (and %v other PCs) during conversion", cc.errPC, cc.errCount)
+	return fmt.Sprintf("%v; not found in module map", errMsg)
+}
+
+func (cc *convertContext) discard(pc uint32) {
+	cc.errCount += 1
+	if cc.errPC == 0 {
+		cc.errPC = pc
+	}
 }
diff --git a/pkg/cover/canonicalizer_test.go b/pkg/cover/canonicalizer_test.go
index c9e208dcc..db418abe1 100644
--- a/pkg/cover/canonicalizer_test.go
+++ b/pkg/cover/canonicalizer_test.go
@@ -204,26 +204,65 @@ func TestModules(t *testing.T) {
 	}
 }
 
+// Tests coverage conversion when modules are added after initialization.
+func TestChangingModules(t *testing.T) {
+	serv := &RPCServer{
+		fuzzers: make(map[string]*Fuzzer),
+	}
+
+	// Create modules at the specified address offsets.
+	f1ModuleAddresses := []uint64{0x00015000}
+	f1ModuleSizes := []uint64{0x5000}
+	f1Modules := initModules(f1ModuleAddresses, f1ModuleSizes)
+	serv.connect("f1", f1Modules, true)
+
+	f2ModuleAddresses := []uint64{0x00015000, 0x00020000}
+	f2ModuleSizes := []uint64{0x5000, 0x5000}
+	f2Modules := initModules(f2ModuleAddresses, f2ModuleSizes)
+	serv.connect("f2", f2Modules, true)
+
+	// Module 2 is not present in the "canonical" fuzzer, so coverage values
+	// in this range should be deleted.
+	serv.fuzzers["f2"].cov = []uint32{0x00010000, 0x00015000, 0x00020000, 0x00025000}
+	serv.fuzzers["f2"].goalCov = []uint32{0x00010000, 0x00015000, 0x00025000}
+	serv.fuzzers["f2"].sign = signal.FromRaw(serv.fuzzers["f2"].cov, 0).Serialize()
+	serv.fuzzers["f2"].goalSign = signal.FromRaw(serv.fuzzers["f2"].goalCov, 0).Serialize()
+
+	if err := serv.runTest(Canonicalize); err != "" {
+		t.Fatalf("failed in canonicalization: %v", err)
+	}
+
+	serv.fuzzers["f2"].goalCov = []uint32{0x00010000, 0x00015000, 0x00025000}
+	serv.fuzzers["f2"].goalSign = signal.FromRaw(serv.fuzzers["f2"].goalCov, 0).Serialize()
+	if err := serv.runTest(Decanonicalize); err != "" {
+		t.Fatalf("failed in decanonicalization: %v", err)
+	}
+}
+
 func (serv *RPCServer) runTest(val canonicalizeValue) string {
+	var cov []uint32
+	var sign signal.Serial
 	for name, fuzzer := range serv.fuzzers {
 		if val == Canonicalize {
-			fuzzer.instModules.Canonicalize(fuzzer.cov, fuzzer.sign)
+			cov, sign = fuzzer.instModules.Canonicalize(fuzzer.cov, fuzzer.sign)
 		} else {
-			fuzzer.instModules.Decanonicalize(fuzzer.cov, fuzzer.sign)
+			cov, sign = fuzzer.instModules.Decanonicalize(fuzzer.cov, fuzzer.sign)
 			instBitmap := fuzzer.instModules.DecanonicalizeFilter(fuzzer.bitmap)
 			if !reflect.DeepEqual(instBitmap, fuzzer.goalBitmap) {
 				return fmt.Sprintf("failed in bitmap conversion. Fuzzer %v.\nExpected: 0x%x.\nReturned: 0x%x",
 					name, fuzzer.goalBitmap, instBitmap)
 			}
 		}
-		if !reflect.DeepEqual(fuzzer.cov, fuzzer.goalCov) {
+		if !reflect.DeepEqual(cov, fuzzer.goalCov) {
 			return fmt.Sprintf("failed in coverage conversion. Fuzzer %v.\nExpected: 0x%x.\nReturned: 0x%x",
-				name, fuzzer.goalCov, fuzzer.cov)
+				name, fuzzer.goalCov, cov)
 		}
-		if !reflect.DeepEqual(fuzzer.sign.Deserialize(), fuzzer.goalSign.Deserialize()) {
+		if !reflect.DeepEqual(sign.Deserialize(), fuzzer.goalSign.Deserialize()) {
 			return fmt.Sprintf("failed in signal conversion. Fuzzer %v.\nExpected: 0x%x.\nReturned: 0x%x",
-				name, fuzzer.goalSign, fuzzer.sign)
+				name, fuzzer.goalSign, sign)
 		}
+		fuzzer.cov = cov
+		fuzzer.sign = sign
 	}
 	return ""
 }
diff --git a/pkg/signal/signal.go b/pkg/signal/signal.go
index 705fab5d7..86c4a4ab9 100644
--- a/pkg/signal/signal.go
+++ b/pkg/signal/signal.go
@@ -79,8 +79,9 @@ func (s Signal) Serialize() Serial {
 	return res
 }
 
-func (ser Serial) UpdateElem(idx int, newElem uint32) {
-	ser.Elems[idx] = elemType(newElem)
+func (ser *Serial) AddElem(elem uint32, prio prioType) {
+	ser.Elems = append(ser.Elems, elemType(elem))
+	ser.Prios = append(ser.Prios, prio)
 }
 
 func (ser Serial) Deserialize() Signal {