From 439ed9617cff358de139af701b3eae0b75c8014e Mon Sep 17 00:00:00 2001
From: Gauvain 'GovanifY' Roussel-Tarbouriech <gauvain@govanify.com>
Date: Thu, 20 Aug 2020 07:33:27 +0200
Subject: [PATCH] IPC: perform memory checks, virtually no perf hit

---
 pcsx2/IPC.cpp | 65 +++++++++++++++++++++++++++------------------------
 pcsx2/IPC.h   | 18 +++++++++++---
 2 files changed, 50 insertions(+), 33 deletions(-)

diff --git a/pcsx2/IPC.cpp b/pcsx2/IPC.cpp
index e48b8b4c7d..0a1de5bb50 100644
--- a/pcsx2/IPC.cpp
+++ b/pcsx2/IPC.cpp
@@ -169,24 +169,24 @@ char* SocketIPC::MakeFailIPC(char* ret_buffer)
 SocketIPC::IPCBuffer SocketIPC::ParseCommand(char* buf, char* ret_buffer)
 {
 	// currently all our instructions require a running VM so we check once
-	// here, will help perf when/if we implement multi-ipc processing in one
-	// socket roundtrip.
+	// here, slightly helps performance
 	if (!m_vm->HasActiveMachine())
 		return IPCBuffer{1, MakeFailIPC(ret_buffer)};
 
 
 	u16 batch = 1;
 	u32 ret_cnt = 1;
+	u32 buf_cnt = 0;
 	if ((IPCCommand)buf[0] == MsgMultiCommand)
 	{
-		batch = FromArray<u16>(buf, 1);
-		buf += 3;
+		batch = FromArray<u16>(&buf[buf_cnt], 1);
+		buf_cnt += 3;
 	}
 
 	for (u16 i = 0; i < batch; i++)
 	{
 		// YY YY YY YY from schema below
-		u32 a = FromArray<u32>(buf, 1);
+		u32 a = FromArray<u32>(&buf[buf_cnt], 1);
 
 		//         IPC Message event (1 byte)
 		//         |  Memory address (4 byte)
@@ -197,82 +197,87 @@ SocketIPC::IPCBuffer SocketIPC::ParseCommand(char* buf, char* ret_buffer)
 		//        |  return value (VLE)
 		//        |  |
 		// reply: XX ZZ ZZ ZZ ZZ
-		//
-		// NB: memory safety checking would be very expansive in our case,
-		// implemented per command and simply a mess. As our threat model is
-		// nonexistant, knowing we can disable the IPC at any time and having
-		// checks client-side it simply makes more sense to not do check
-		// server-side, as bad as this sounds.
-		// Re security threat model: we control the entire emulated memory of
-		// the emulated game, we can DoS our emulator easily by abusing the IPC
-		// features already, and regardless of where you read this there's
-		// probably no sandbox implemented, so simply being able to write to the
-		// game memory code region would make us control the JIT and have probably
-		// full access over the host machine.
-		switch ((IPCCommand)buf[0])
+		switch ((IPCCommand)buf[buf_cnt])
 		{
 			case MsgRead8:
 			{
+				if (!SafetyChecks(buf_cnt, 5, ret_cnt, 1))
+					goto error;
 				u8 res;
 				res = memRead8(a);
 				ToArray(ret_buffer, res, ret_cnt);
 				ret_cnt += 1;
-				buf += 5;
+				buf_cnt += 5;
 				break;
 			}
 			case MsgRead16:
 			{
+				if (!SafetyChecks(buf_cnt, 5, ret_cnt, 2))
+					goto error;
 				u16 res;
 				res = memRead16(a);
 				ToArray(ret_buffer, res, ret_cnt);
 				ret_cnt += 2;
-				buf += 5;
+				buf_cnt += 5;
 				break;
 			}
 			case MsgRead32:
 			{
+				if (!SafetyChecks(buf_cnt, 5, ret_cnt, 4))
+					goto error;
 				u32 res;
 				res = memRead32(a);
 				ToArray(ret_buffer, res, ret_cnt);
 				ret_cnt += 4;
-				buf += 5;
+				buf_cnt += 5;
 				break;
 			}
 			case MsgRead64:
 			{
+				if (!SafetyChecks(buf_cnt, 5, ret_cnt, 8))
+					goto error;
 				u64 res;
 				memRead64(a, &res);
 				ToArray(ret_buffer, res, ret_cnt);
 				ret_cnt += 8;
-				buf += 5;
+				buf_cnt += 5;
 				break;
 			}
 			case MsgWrite8:
 			{
-				memWrite8(a, FromArray<u8>(buf, 5));
-				buf += 6;
+				if (!SafetyChecks(buf_cnt, 6, ret_cnt))
+					goto error;
+				memWrite8(a, FromArray<u8>(&buf[buf_cnt], 5));
+				buf_cnt += 6;
 				break;
 			}
 			case MsgWrite16:
 			{
-				memWrite16(a, FromArray<u16>(buf, 5));
-				buf += 7;
+				if (!SafetyChecks(buf_cnt, 7, ret_cnt))
+					goto error;
+				memWrite16(a, FromArray<u16>(&buf[buf_cnt], 5));
+				buf_cnt += 7;
 				break;
 			}
 			case MsgWrite32:
 			{
-				memWrite32(a, FromArray<u32>(buf, 5));
-				buf += 9;
+				if (!SafetyChecks(buf_cnt, 9, ret_cnt))
+					goto error;
+				memWrite32(a, FromArray<u32>(&buf[buf_cnt], 5));
+				buf_cnt += 9;
 				break;
 			}
 			case MsgWrite64:
 			{
-				memWrite64(a, FromArray<u64>(buf, 5));
-				buf += 13;
+				if (!SafetyChecks(buf_cnt, 13, ret_cnt))
+					goto error;
+				memWrite64(a, FromArray<u64>(&buf[buf_cnt], 5));
+				buf_cnt += 13;
 				break;
 			}
 			default:
 			{
+			error:
 				return IPCBuffer{1, MakeFailIPC(ret_buffer)};
 			}
 		}
diff --git a/pcsx2/IPC.h b/pcsx2/IPC.h
index ad3248e03f..c5749a02d6 100644
--- a/pcsx2/IPC.h
+++ b/pcsx2/IPC.h
@@ -42,18 +42,17 @@ protected:
 	int m_sock = 0;
 #endif
 
-
 	/**
      * Maximum memory used by an IPC message request.
      * Equivalent to 50,000 Write64 requests.
      */
-	const unsigned int MAX_IPC_SIZE = 650000;
+#define MAX_IPC_SIZE 650000
 
 	/**
      * Maximum memory used by an IPC message reply.
      * Equivalent to 50,000 Read64 replies.
      */
-	const unsigned int MAX_IPC_RETURN_SIZE = 450000;
+#define MAX_IPC_RETURN_SIZE 450000
 
 	/**
      * IPC return buffer.
@@ -154,6 +153,19 @@ protected:
 		return *(T*)(arr + i);
 	}
 
+	/**
+     * Ensures an IPC message isn't too big.
+     * return value: false if checks failed, true otherwise.
+     */
+	static inline bool SafetyChecks(u32 command_len, int command_size, u32 reply_len, int reply_size = 0)
+	{
+		bool res = ((command_len + command_size) >= MAX_IPC_SIZE ||
+					(reply_len + reply_size) >= MAX_IPC_RETURN_SIZE);
+		if (unlikely(res))
+			return false;
+		return true;
+	}
+
 public:
 	/* Initializers */
 	SocketIPC(SysCoreThread* vm);