Merge remote-tracking branch

'origin/GP-1179_emteere_PR-1565_marpie_improve_pe_cfg--SQUASHED' into
patch (Closes #1547, Closes #1565)

Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/ControlFlowGuard.java

@@ -15,11 +15,15 @@
  */
 package ghidra.app.util.bin.format.pe;
 
+import java.util.ArrayList;
+import java.util.List;
+
+import ghidra.app.cmd.data.CreateArrayCmd;
 import ghidra.app.util.bin.format.pe.LoadConfigDirectory.GuardFlags;
 import ghidra.app.util.importer.MessageLog;
+import ghidra.app.util.opinion.AbstractProgramLoader;
 import ghidra.program.model.address.*;
-import ghidra.program.model.data.DataType;
+import ghidra.program.model.data.*;
-import ghidra.program.model.data.ImageBaseOffset32DataType;
 import ghidra.program.model.listing.Data;
 import ghidra.program.model.listing.Program;
 import ghidra.program.model.mem.Memory;
@@ -37,6 +41,9 @@ import ghidra.util.exception.InvalidInputException;
  * Creator's update. 
  */
 public class ControlFlowGuard {
+	public static String GuardCFFunctionTableName = "GuardCFFunctionTable";
+	public static String GuardCFAddressTakenIatTableName = "GuardCFAddressTakenIatTable";
+	public static String GuardCfgTableEntryName = "GuardCfgTableEntry";
 
 	/**
 	 * Perform markup on the supported ControlFlowGuard and ReturnFlowGuard functions and 
@@ -56,28 +63,29 @@ public class ControlFlowGuard {
 		SymbolTable symbolTable = program.getSymbolTable();
 
 		// ControlFlowGuard
-		markupCfgCheckFunction(lcd, is64bit, space, mem, symbolTable);
+		markupCfgCheckFunction(lcd, program, is64bit, space, mem, symbolTable);
-		markupCfgDispatchFunction(lcd, is64bit, space, mem, symbolTable);
+		markupCfgDispatchFunction(lcd, program, is64bit, space, mem, symbolTable);
 		markupCfgFunctionTable(lcd, program, log);
-		
+		markupCfgAddressTakenIatEntryTable(lcd, program, log);
+
 		// ReturnFlowGuard
-		markupRfgFailureRoutine(lcd, space, symbolTable);
+		markupRfgFailureRoutine(lcd, program, space, symbolTable);
-		markupRfgDefaultFailureRoutine(lcd, is64bit, space, mem, symbolTable);
+		markupRfgDefaultFailureRoutine(lcd, program, is64bit, space, mem, symbolTable);
-		markupRfgDefaultStackPointerFunction(lcd, is64bit, space, mem, symbolTable);
+		markupRfgDefaultStackPointerFunction(lcd, program, is64bit, space, mem, symbolTable);
 	}
 
 	/**
 	 * Performs markup on the ControlFlowGuard check function, if it exists.
 	 * 
 	 * @param lcd The PE LoadConfigDirectory.
+	 * @param program The program.
 	 * @param is64bit True if the PE is 64-bit; false if it's 32-bit.
 	 * @param space The program's address space.
 	 * @param mem The program's memory.
 	 * @param symbolTable The program's symbol table.
 	 */
-	private static void markupCfgCheckFunction(LoadConfigDirectory lcd, boolean is64bit,
+	private static void markupCfgCheckFunction(LoadConfigDirectory lcd, Program program,
-			AddressSpace space,
+			boolean is64bit, AddressSpace space, Memory mem, SymbolTable symbolTable) {
-			Memory mem, SymbolTable symbolTable) {
 
 		if (lcd.getCfgCheckFunctionPointer() == 0) {
 			return;
@@ -88,6 +96,8 @@ public class ControlFlowGuard {
 			Address functionAddr = space.getAddress(
 				is64bit ? mem.getLong(functionPointerAddr) : mem.getInt(functionPointerAddr));
 			symbolTable.createLabel(functionAddr, "_guard_check_icall", SourceType.IMPORTED);
+
+			AbstractProgramLoader.markAsFunction(program, null, functionAddr);
 		}
 		catch (MemoryAccessException | AddressOutOfBoundsException | InvalidInputException e) {
 			Msg.warn(ControlFlowGuard.class, "Unable to label ControlFlowGuard check function.", e);
@@ -98,13 +108,14 @@ public class ControlFlowGuard {
 	 * Performs markup on the ControlFlowGuard dispatch function, if it exists.
 	 * 
 	 * @param lcd The PE LoadConfigDirectory.
+	 * @param program The program.
 	 * @param is64bit True if the PE is 64-bit; false if it's 32-bit.
 	 * @param space The program's address space.
 	 * @param mem The program's memory.
 	 * @param symbolTable The program's symbol table.
 	 */
-	private static void markupCfgDispatchFunction(LoadConfigDirectory lcd, boolean is64bit,
+	private static void markupCfgDispatchFunction(LoadConfigDirectory lcd, Program program,
-			AddressSpace space, Memory mem, SymbolTable symbolTable) {
+			boolean is64bit, AddressSpace space, Memory mem, SymbolTable symbolTable) {
 
 		if (lcd.getCfgDispatchFunctionPointer() == 0) {
 			return;
@@ -115,6 +126,8 @@ public class ControlFlowGuard {
 			Address functionAddr = space.getAddress(
 				is64bit ? mem.getLong(functionPointerAddr) : mem.getInt(functionPointerAddr));
 			symbolTable.createLabel(functionAddr, "_guard_dispatch_icall", SourceType.IMPORTED);
+
+			AbstractProgramLoader.markAsFunction(program, null, functionAddr);
 		}
 		catch (MemoryAccessException | AddressOutOfBoundsException | InvalidInputException e) {
 			Msg.warn(ControlFlowGuard.class, "Unable to label ControlFlowGuard dispatch function.",
@@ -142,22 +155,110 @@ public class ControlFlowGuard {
 			return;
 		}
 
+		Address tableAddr =
+			program.getAddressFactory().getDefaultAddressSpace().getAddress(tablePointer);
+
+		// Label the start of the table
+		try {
+			program.getSymbolTable()
+					.createLabel(tableAddr, GuardCFFunctionTableName, SourceType.IMPORTED);
+		}
+		catch (InvalidInputException e) {
+			Msg.warn(ControlFlowGuard.class, "Unable to label ControlFlowGuard function table.", e);
+		}
+
+		// Each table entry is an RVA (32-bit image base offset), followed by 'n' extra bytes
+		GuardFlags guardFlags = lcd.getCfgGuardFlags();
+		int n = (guardFlags.getFlags() &
+			IMAGE_GUARD_CF_FUNCTION_TABLE_SIZE_MASK) >> IMAGE_GUARD_CF_FUNCTION_TABLE_SIZE_SHIFT;
+
+		// Pre-define base data types used to define table entry data type
+		DataType ibo32 = new ImageBaseOffset32DataType();
+		DataType byteType = ByteDataType.dataType;
+
+		CategoryPath categoryPath = new CategoryPath(CategoryPath.ROOT, "CFG");
+		StructureDataType GuardCfgTableEntryType = (StructureDataType) program.getDataTypeManager()
+				.getDataType(categoryPath, GuardCfgTableEntryName);
+
+		if (GuardCfgTableEntryType == null) {
+			GuardCfgTableEntryType = new StructureDataType(categoryPath, GuardCfgTableEntryName, 0);
+			GuardCfgTableEntryType.setPackingEnabled(false);
+			GuardCfgTableEntryType.add(ibo32, "Offset", "");
+			if (n > 0) {
+				ArrayDataType padType =
+					new ArrayDataType(byteType, n / byteType.getLength(), byteType.getLength());
+				GuardCfgTableEntryType.add(padType, "Pad", "");
+			}
+		}
+
+		CreateArrayCmd cmd = new CreateArrayCmd(tableAddr, (int) functionCount,
+			GuardCfgTableEntryType, GuardCfgTableEntryType.getLength());
+		cmd.applyTo(program);
+
+		Data tableData = program.getListing().getDataAt(tableAddr);
+		createCfgFunctions(program, tableData, log);
+	}
+
+	private static void createCfgFunctions(Program program, Data tableData, MessageLog log) {
+		if (tableData == null) {
+			Msg.warn(ControlFlowGuard.class, "Couldn't find Control Flow Guard tables.");
+			return;
+		}
+
+		if (!tableData.isArray() || (tableData.getNumComponents() < 1)) {
+			Msg.warn(ControlFlowGuard.class, "Control Flow Guard table seems to be empty.");
+			return;
+		}
+
+		for (Address target : getFunctionAddressesFromTable(program, tableData)) {
+			AbstractProgramLoader.markAsFunction(program, null, target);
+		}
+	}
+
+	private static List<Address> getFunctionAddressesFromTable(Program program, Data table) {
+		List<Address> list = new ArrayList<Address>();
+
+		// use the array and ibo data in structure to get a list of functions
+		for (int i = 0; i < table.getNumComponents(); i++) {
+			Data entry = table.getComponent(i);
+			Data iboData = entry.getComponent(0);
+			Object value = iboData.getValue();
+			if (value instanceof Address) {
+				list.add((Address) value);
+			}
+		}
+		return list;
+	}
+
+	/**
+	 * Performs markup on the ControlFlowGuard address taken IAT table, if it exists.
+	 * 
+	 * @param lcd The PE LoadConfigDirectory.
+	 * @param program The program.
+	 * @param log The log.
+	 */
+	private static void markupCfgAddressTakenIatEntryTable(LoadConfigDirectory lcd, Program program,
+			MessageLog log) {
+
+		long tablePointer = lcd.getGuardAddressIatTableTablePointer();
+		long functionCount = lcd.getGuardAddressIatTableCount();
+
+		if (tablePointer == 0 || functionCount <= 0) {
+			return;
+		}
+
 		try {
 			Address tableAddr =
 				program.getAddressFactory().getDefaultAddressSpace().getAddress(tablePointer);
 
 			// Label the start of the table
-			program.getSymbolTable().createLabel(tableAddr, "GuardCFFunctionTable",
+			program.getSymbolTable()
-				SourceType.IMPORTED);
+					.createLabel(tableAddr, GuardCFAddressTakenIatTableName, SourceType.IMPORTED);
-
+			// Each table entry is an RVA (32-bit image base offset)
-			// Each table entry is an RVA (32-bit image base offset), followed by 'n' extra bytes
-			GuardFlags guardFlags = lcd.getCfgGuardFlags();
-			int n = (guardFlags.getFlags() &
-				IMAGE_GUARD_CF_FUNCTION_TABLE_SIZE_MASK) >> IMAGE_GUARD_CF_FUNCTION_TABLE_SIZE_SHIFT;
 			DataType ibo32 = new ImageBaseOffset32DataType();
 			for (long i = 0; i < functionCount; i++) {
-				Data d = PeUtils.createData(program, tableAddr.add(i * (ibo32.getLength() + n)),
+				Data d =
-					ibo32, log);
+					PeUtils.createData(program, tableAddr.add(i * ibo32.getLength()), ibo32, log);
 				if (d == null) {
 					// If we failed to create data on a table entry, just assume the rest will fail
 					break;
@@ -165,7 +266,7 @@ public class ControlFlowGuard {
 			}
 		}
 		catch (AddressOutOfBoundsException | InvalidInputException e) {
-			Msg.warn(ControlFlowGuard.class, "Unable to label ControlFlowGuard function table.", e);
+			Msg.warn(ControlFlowGuard.class, "Unable to label ControlFlowGuard IAT table.", e);
 		}
 	}
 
@@ -173,11 +274,12 @@ public class ControlFlowGuard {
 	 * Performs markup on the ReturnFlowGuard failure routine, if it exists.
 	 * 
 	 * @param lcd The PE LoadConfigDirectory.
+	 * @param program The program
 	 * @param space The program's address space.
 	 * @param symbolTable The program's symbol table.
 	 */
-	private static void markupRfgFailureRoutine(LoadConfigDirectory lcd, AddressSpace space,
+	private static void markupRfgFailureRoutine(LoadConfigDirectory lcd, Program program,
-			SymbolTable symbolTable) {
+			AddressSpace space, SymbolTable symbolTable) {
 
 		if (lcd.getRfgFailureRoutine() == 0) {
 			return;
@@ -186,6 +288,8 @@ public class ControlFlowGuard {
 		try {
 			Address routineAddr = space.getAddress(lcd.getRfgFailureRoutine());
 			symbolTable.createLabel(routineAddr, "_guard_ss_verify_failure", SourceType.IMPORTED);
+
+			AbstractProgramLoader.markAsFunction(program, null, routineAddr);
 		}
 		catch (AddressOutOfBoundsException | InvalidInputException e) {
 			Msg.warn(ControlFlowGuard.class, "Unable to label ReturnFlowGuard failure routine.", e);
@@ -196,13 +300,14 @@ public class ControlFlowGuard {
 	 * Performs markup on the ReturnFlowGuard "default" failure routine function, if it exists.
 	 * 
 	 * @param lcd The PE LoadConfigDirectory.
+	 * @param program The program
 	 * @param is64bit True if the PE is 64-bit; false if it's 32-bit.
 	 * @param space The program's address space.
 	 * @param mem The program's memory.
 	 * @param symbolTable The program's symbol table.
 	 */
-	private static void markupRfgDefaultFailureRoutine(LoadConfigDirectory lcd, boolean is64bit,
+	private static void markupRfgDefaultFailureRoutine(LoadConfigDirectory lcd, Program program,
-			AddressSpace space, Memory mem, SymbolTable symbolTable) {
+			boolean is64bit, AddressSpace space, Memory mem, SymbolTable symbolTable) {
 
 		if (lcd.getRfgFailureRoutineFunctionPointer() == 0) {
 			return;
@@ -215,6 +320,9 @@ public class ControlFlowGuard {
 				is64bit ? mem.getLong(functionPointerAddr) : mem.getInt(functionPointerAddr));
 			symbolTable.createLabel(functionAddr, "_guard_ss_verify_failure_default",
 				SourceType.IMPORTED);
+
+			AbstractProgramLoader.markAsFunction(program, null, functionAddr);
+
 		}
 		catch (MemoryAccessException | AddressOutOfBoundsException | InvalidInputException e) {
 			Msg.warn(ControlFlowGuard.class,
@@ -226,13 +334,15 @@ public class ControlFlowGuard {
 	 * Performs markup on the ReturnFlowGuard verify stack pointer function, if it exists.
 	 * 
 	 * @param lcd The PE LoadConfigDirectory.
+	 * @param program The program
 	 * @param is64bit True if the PE is 64-bit; false if it's 32-bit.
 	 * @param space The program's address space.
 	 * @param mem The program's memory.
 	 * @param symbolTable The program's symbol table.
 	 */
 	private static void markupRfgDefaultStackPointerFunction(LoadConfigDirectory lcd,
-			boolean is64bit, AddressSpace space, Memory mem, SymbolTable symbolTable) {
+			Program program, boolean is64bit, AddressSpace space, Memory mem,
+			SymbolTable symbolTable) {
 
 		if (lcd.getRfgVerifyStackPointerFunctionPointer() == 0) {
 			return;
@@ -245,6 +355,9 @@ public class ControlFlowGuard {
 				is64bit ? mem.getLong(functionPointerAddr) : mem.getInt(functionPointerAddr));
 			symbolTable.createLabel(functionAddr, "_guard_ss_verify_sp_default",
 				SourceType.IMPORTED);
+
+			AbstractProgramLoader.markAsFunction(program, null, functionAddr);
+
 		}
 		catch (MemoryAccessException | AddressOutOfBoundsException | InvalidInputException e) {
 			Msg.warn(ControlFlowGuard.class,

Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/pe/LoadConfigDirectory.java

@@ -254,6 +254,24 @@ public class LoadConfigDirectory implements StructConverter {
 		return guardCfFunctionCount;
 	}
 
+	/**
+	 * Gets the ControlFlowGuard IAT table pointer address.
+	 * 
+	 * @return The ControlFlowGuard IAT table function pointer address. Could be 0 if ControlFlowGuard is not being used
+	 */
+	public long getGuardAddressIatTableTablePointer() {
+		return guardAddressTakenIatEntryTable;
+	}
+
+	/**
+	 * Gets the ControlFlowGuard IAT entries count.
+	 * 
+	 * @return The ControlFlowGuard IAT entries count.  Could be 0 if ControlFlowGuard is not being used
+	 */
+	public long getGuardAddressIatTableCount() {
+		return guardAddressTakenIatEntryCount;
+	}
+
 	/**
 	 * Gets the ReturnFlowGuard failure routine address.
 	 * 

Ghidra/Features/Base/src/main/java/ghidra/app/util/opinion/AbstractProgramLoader.java

@@ -29,10 +29,10 @@ import ghidra.framework.model.DomainFolder;
 import ghidra.framework.model.DomainObject;
 import ghidra.framework.store.LockException;
 import ghidra.program.database.ProgramDB;
+import ghidra.program.database.function.OverlappingFunctionException;
 import ghidra.program.model.address.*;
 import ghidra.program.model.lang.*;
-import ghidra.program.model.listing.Function;
+import ghidra.program.model.listing.*;
-import ghidra.program.model.listing.Program;
 import ghidra.program.model.mem.MemoryConflictException;
 import ghidra.program.model.symbol.*;
 import ghidra.program.model.util.AddressLabelInfo;
@@ -347,6 +347,33 @@ public abstract class AbstractProgramLoader implements Loader {
 		}
 	}
 
+	/**
+	 * Mark this address as a function by creating a one byte function.  The single byte body
+	 * function is picked up by the function analyzer, disassembled, and the body fixed.
+	 * Marking the function this way keeps disassembly and follow on analysis out of the loaders.
+	 * 
+	 * @param program the program
+	 * @param name name of function, null if name not known
+	 * @param funcStart starting address of the function
+	 */
+	public static void markAsFunction(Program program, String name, Address funcStart) {
+		FunctionManager functionMgr = program.getFunctionManager();
+
+		if (functionMgr.getFunctionAt(funcStart) != null) {
+			return;
+		}
+		try {
+			functionMgr.createFunction(name, funcStart, new AddressSet(funcStart, funcStart),
+				SourceType.IMPORTED);
+		}
+		catch (InvalidInputException e) {
+			// ignore
+		}
+		catch (OverlappingFunctionException e) {
+			// ignore
+		}
+	}
+
 	/**
 	 * Gets the {@link Loader}'s language service.
 	 * <p>

Ghidra/Features/Base/src/main/java/ghidra/app/util/opinion/CoffLoader.java

@@ -28,7 +28,6 @@ import ghidra.app.util.bin.format.coff.relocation.CoffRelocationHandler;
 import ghidra.app.util.bin.format.coff.relocation.CoffRelocationHandlerFactory;
 import ghidra.app.util.importer.MessageLog;
 import ghidra.framework.model.DomainObject;
-import ghidra.program.database.function.OverlappingFunctionException;
 import ghidra.program.database.mem.FileBytes;
 import ghidra.program.model.address.*;
 import ghidra.program.model.data.*;
@@ -153,7 +152,8 @@ public class CoffLoader extends AbstractLibrarySupportLoader {
 	}
 
 	@Override
-	public String validateOptions(ByteProvider provider, LoadSpec loadSpec, List<Option> options, Program program) {
+	public String validateOptions(ByteProvider provider, LoadSpec loadSpec, List<Option> options,
+			Program program) {
 		if (options != null) {
 			for (Option option : options) {
 				String name = option.getName();
@@ -277,8 +277,8 @@ public class CoffLoader extends AbstractLibrarySupportLoader {
 
 					symbolsMap.put(symbol, sym);
 
-					externalAddress = externalAddress.add(
+					externalAddress = externalAddress
-						getPointerSizeAligned(externalAddress.getAddressSpace()));
+							.add(getPointerSizeAligned(externalAddress.getAddressSpace()));
 				}
 				else if (sectionNum <= -2) {
 					log.appendMsg("Strange symbol " + symbol + " : " + symbol.getBasicType() +
@@ -342,7 +342,7 @@ public class CoffLoader extends AbstractLibrarySupportLoader {
 						symbol.getStorageClass() != CoffSymbolStorageClass.C_STAT) {
 						// ONLY DO THIS IF THE SYMBOL IS A FUNCTION!
 						symbolTable.addExternalEntryPoint(address);
-						createOneByteFunction(program, sym.getName(), address);
+						markAsFunction(program, sym.getName(), address);
 					}
 
 					symbolsMap.put(symbol, sym);
@@ -365,7 +365,7 @@ public class CoffLoader extends AbstractLibrarySupportLoader {
 			try {
 				MemoryBlock block = program.getMemory()
 						.createUninitializedBlock(MemoryBlock.EXTERNAL_BLOCK_NAME,
-					externalAddressStart, size, false);
+							externalAddressStart, size, false);
 
 				// assume any value in external is writable.
 				block.setWrite(true);
@@ -470,8 +470,8 @@ public class CoffLoader extends AbstractLibrarySupportLoader {
 				// don't create a block, but record the section to get at the address!
 				block = program.getMemory().getBlock(sectionAddr);
 				try {
-					program.getSymbolTable().createLabel(sectionAddr, section.getName(),
+					program.getSymbolTable()
-						SourceType.IMPORTED);
+							.createLabel(sectionAddr, section.getName(), SourceType.IMPORTED);
 					// TODO: sectionSize somewhere for case where flags==0 ?
 				}
 				catch (InvalidInputException e) {
@@ -702,36 +702,21 @@ public class CoffLoader extends AbstractLibrarySupportLoader {
 					}
 				}
 
-				program.getRelocationTable().add(address, relocation.getType(),
+				program.getRelocationTable()
-					new long[] { relocation.getSymbolIndex() }, origBytes,
+						.add(address, relocation.getType(),
-					symbol != null ? symbol.getName() : "<null>");
+							new long[] { relocation.getSymbolIndex() }, origBytes,
+							symbol != null ? symbol.getName() : "<null>");
 			}
 		}
 	}
 
 	private void handleRelocationError(Program program, MessageLog log, Address address,
 			String message) {
-		program.getBookmarkManager().setBookmark(address, BookmarkType.ERROR, "Relocations",
+		program.getBookmarkManager()
-			message);
+				.setBookmark(address, BookmarkType.ERROR, "Relocations", message);
 		log.appendMsg(message);
 	}
 
-	private void createOneByteFunction(Program program, String name, Address address) {
-		FunctionManager functionMgr = program.getFunctionManager();
-		if (functionMgr.getFunctionAt(address) != null) {
-			return;
-		}
-		try {
-			functionMgr.createFunction(name, address, new AddressSet(address), SourceType.IMPORTED);
-		}
-		catch (InvalidInputException e) {
-			// ignore
-		}
-		catch (OverlappingFunctionException e) {
-			// ignore
-		}
-	}
-
 	@Override
 	public String getName() {
 		return COFF_NAME;