From 0b91aa50eeb80fbb2999c542342a908736b28d65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 5 May 2024 12:23:43 +0200 Subject: [PATCH] Improve memory safety, add bugfix to little-used sysclib functions --- Core/HLE/sceIo.cpp | 23 +++++++++++++++++------ Core/HLE/sceKernelInterrupt.cpp | 28 ++++++++++++++-------------- Core/HLE/sceKernelModule.cpp | 2 +- Core/MemMap.h | 13 +++++++------ 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/Core/HLE/sceIo.cpp b/Core/HLE/sceIo.cpp index d370f81438..974a224a26 100644 --- a/Core/HLE/sceIo.cpp +++ b/Core/HLE/sceIo.cpp @@ -838,6 +838,12 @@ static void IoStartAsyncThread(int id, FileNode *f) { static u32 sceIoAssign(u32 alias_addr, u32 physical_addr, u32 filesystem_addr, int mode, u32 arg_addr, int argSize) { + if (!Memory::IsValidNullTerminatedString(alias_addr) || + !Memory::IsValidNullTerminatedString(physical_addr) || + !Memory::IsValidNullTerminatedString(filesystem_addr)) { + return hleLogError(SCEIO, -1, "Bad parameters"); + } + std::string alias = Memory::GetCharPointer(alias_addr); std::string physical_dev = Memory::GetCharPointer(physical_addr); std::string filesystem_dev = Memory::GetCharPointer(filesystem_addr); @@ -2009,8 +2015,7 @@ static u32 sceIoDevctl(const char *name, int cmd, u32 argAddr, int argLen, u32 o { // Emulator special tricks! - enum - { + enum { EMULATOR_DEVCTL__GET_HAS_DISPLAY = 1, EMULATOR_DEVCTL__SEND_OUTPUT, EMULATOR_DEVCTL__IS_EMULATOR, @@ -2031,14 +2036,14 @@ static u32 sceIoDevctl(const char *name, int cmd, u32 argAddr, int argLen, u32 o Memory::Write_U32(PSP_CoreParameter().headLess ? 0 : 1, outPtr); return 0; case EMULATOR_DEVCTL__SEND_OUTPUT: - { - std::string data(Memory::GetCharPointer(argAddr), argLen); + if (Memory::IsValidRange(argAddr, argLen)) { + std::string data(Memory::GetCharPointerUnchecked(argAddr), argLen); if (!System_SendDebugOutput(data)) DEBUG_LOG(SCEIO, "%s", data.c_str()); if (PSP_CoreParameter().collectDebugOutput) *PSP_CoreParameter().collectDebugOutput += data; - return 0; } + return 0; case EMULATOR_DEVCTL__IS_EMULATOR: if (Memory::IsValidAddress(outPtr)) Memory::Write_U32(1, outPtr); @@ -2932,7 +2937,13 @@ static int IoAsyncFinish(int id) { case IoAsyncOp::OPEN: { // See notes on timing in sceIoOpen. - const std::string filename = Memory::GetCharPointer(params.open.filenameAddr); + if (!Memory::IsValidNullTerminatedString(params.open.filenameAddr)) { + // Bad + ERROR_LOG(SCEIO, "Bad pointer to filename %08x", params.open.filenameAddr); + us = 80; + break; + } + const std::string filename = Memory::GetCharPointerUnchecked(params.open.filenameAddr); IFileSystem *sys = pspFileSystem.GetSystemFromFilename(filename); if (sys) { if (f->asyncResult == (int)SCE_KERNEL_ERROR_ERRNO_FILE_NOT_FOUND) { diff --git a/Core/HLE/sceKernelInterrupt.cpp b/Core/HLE/sceKernelInterrupt.cpp index 85e2bf425c..33dcf2f303 100644 --- a/Core/HLE/sceKernelInterrupt.cpp +++ b/Core/HLE/sceKernelInterrupt.cpp @@ -697,7 +697,7 @@ static u32 sysclib_memcpy(u32 dst, u32 src, u32 size) { static u32 sysclib_strcat(u32 dst, u32 src) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strcat(dest=%08x, src=%08x)", dst, src); - if (Memory::IsValidAddress(dst) && Memory::IsValidAddress(src)) { + if (Memory::IsValidNullTerminatedString(dst) && Memory::IsValidNullTerminatedString(src)) { strcat((char *)Memory::GetPointerWriteUnchecked(dst), (const char *)Memory::GetPointerUnchecked(src)); } return dst; @@ -705,7 +705,7 @@ static u32 sysclib_strcat(u32 dst, u32 src) { static int sysclib_strcmp(u32 dst, u32 src) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strcmp(dest=%08x, src=%08x)", dst, src); - if (Memory::IsValidAddress(dst) && Memory::IsValidAddress(src)) { + if (Memory::IsValidNullTerminatedString(dst) && Memory::IsValidNullTerminatedString(src)) { return strcmp((const char *)Memory::GetPointerUnchecked(dst), (const char *)Memory::GetPointerUnchecked(src)); } else { // What to do? Crash, probably. @@ -715,7 +715,7 @@ static int sysclib_strcmp(u32 dst, u32 src) { static u32 sysclib_strcpy(u32 dst, u32 src) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strcpy(dest=%08x, src=%08x)", dst, src); - if (Memory::IsValidAddress(dst) && Memory::IsValidAddress(src)) { + if (Memory::IsValidAddress(dst) && Memory::IsValidNullTerminatedString(src)) { strcpy((char *)Memory::GetPointerWriteUnchecked(dst), (const char *)Memory::GetPointerUnchecked(src)); } return dst; @@ -723,7 +723,7 @@ static u32 sysclib_strcpy(u32 dst, u32 src) { static u32 sysclib_strlen(u32 src) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strlen(src=%08x)", src); - if (Memory::IsValidAddress(src)) { + if (Memory::IsValidNullTerminatedString(src)) { // TODO: This computes the length, could reuse it maybe. return (u32)strlen(Memory::GetCharPointerUnchecked(src)); } else { // What to do? Crash, probably. @@ -893,7 +893,7 @@ static int sysclib_sprintf(u32 dst, u32 fmt) { } static u32 sysclib_memset(u32 destAddr, int data, int size) { - ERROR_LOG(SCEKERNEL, "Untested sysclib_memset(dest=%08x, data=%d ,size=%d)", destAddr, data, size); + DEBUG_LOG(SCEKERNEL, "Untested sysclib_memset(dest=%08x, data=%d ,size=%d)", destAddr, data, size); if (Memory::IsValidRange(destAddr, size)) { memset(Memory::GetPointerWriteUnchecked(destAddr), data, size); } @@ -902,8 +902,8 @@ static u32 sysclib_memset(u32 destAddr, int data, int size) { } static int sysclib_strstr(u32 s1, u32 s2) { - ERROR_LOG(SCEKERNEL, "Untested sysclib_strstr(%08x, %08x)", s1, s2); - if (Memory::IsValidAddress(s1) && Memory::IsValidAddress(s2)) { + DEBUG_LOG(SCEKERNEL, "Untested sysclib_strstr(%08x, %08x)", s1, s2); + if (Memory::IsValidNullTerminatedString(s1) && Memory::IsValidNullTerminatedString(s2)) { std::string str1 = Memory::GetCharPointerUnchecked(s1); std::string str2 = Memory::GetCharPointerUnchecked(s2); size_t index = str1.find(str2); @@ -916,8 +916,8 @@ static int sysclib_strstr(u32 s1, u32 s2) { } static int sysclib_strncmp(u32 s1, u32 s2, u32 size) { - ERROR_LOG(SCEKERNEL, "Untested sysclib_strncmp(%08x, %08x, %08x)", s1, s2, size); - if (Memory::IsValidAddress(s1) && Memory::IsValidAddress(s2)) { + DEBUG_LOG(SCEKERNEL, "Untested sysclib_strncmp(%08x, %08x, %08x)", s1, s2, size); + if (Memory::IsValidRange(s1, size) && Memory::IsValidRange(s2, size)) { const char * str1 = Memory::GetCharPointerUnchecked(s1); const char * str2 = Memory::GetCharPointerUnchecked(s2); return strncmp(str1, str2, size); @@ -926,7 +926,7 @@ static int sysclib_strncmp(u32 s1, u32 s2, u32 size) { } static u32 sysclib_memmove(u32 dst, u32 src, u32 size) { - ERROR_LOG(SCEKERNEL, "Untested sysclib_memmove(%08x, %08x, %08x)", dst, src, size); + DEBUG_LOG(SCEKERNEL, "Untested sysclib_memmove(%08x, %08x, %08x)", dst, src, size); if (Memory::IsValidRange(dst, size) && Memory::IsValidRange(src, size)) { memmove(Memory::GetPointerWriteUnchecked(dst), Memory::GetPointerUnchecked(src), size); } @@ -937,7 +937,7 @@ static u32 sysclib_memmove(u32 dst, u32 src, u32 size) { } static u32 sysclib_strncpy(u32 dest, u32 src, u32 size) { - if (!Memory::IsValidAddress(dest) || Memory::IsValidAddress(src)) { + if (!Memory::IsValidAddress(dest) || !Memory::IsValidAddress(src)) { return hleLogError(SCEKERNEL, 0, "invalid address"); } @@ -962,7 +962,7 @@ static u32 sysclib_strncpy(u32 dest, u32 src, u32 size) { } static u32 sysclib_strtol(u32 strPtr, u32 endPtrPtr, int base) { - if (!Memory::IsValidAddress(strPtr)) { + if (!Memory::IsValidNullTerminatedString(strPtr)) { return hleLogError(SCEKERNEL, 0, "invalid address"); } const char* str = Memory::GetCharPointer(strPtr); @@ -974,7 +974,7 @@ static u32 sysclib_strtol(u32 strPtr, u32 endPtrPtr, int base) { } static u32 sysclib_strchr(u32 src, int c) { - if (!Memory::IsValidAddress(src)) { + if (!Memory::IsValidNullTerminatedString(src)) { return hleLogError(SCEKERNEL, 0, "invalid address"); } const std::string str = Memory::GetCharPointer(src); @@ -986,7 +986,7 @@ static u32 sysclib_strchr(u32 src, int c) { } static u32 sysclib_strrchr(u32 src, int c) { - if (!Memory::IsValidAddress(src)) { + if (!Memory::IsValidNullTerminatedString(src)) { return hleLogError(SCEKERNEL, 0, "invalid address"); } const std::string str = Memory::GetCharPointer(src); diff --git a/Core/HLE/sceKernelModule.cpp b/Core/HLE/sceKernelModule.cpp index bd61e25b75..409698cea8 100644 --- a/Core/HLE/sceKernelModule.cpp +++ b/Core/HLE/sceKernelModule.cpp @@ -1814,7 +1814,7 @@ bool __KernelLoadExec(const char *filename, u32 paramPtr, std::string *error_str } if (param.keyp != 0) { u32 keyAddr = param.keyp; - size_t keylen = strlen(Memory::GetCharPointer(keyAddr))+1; + size_t keylen = strlen(Memory::GetCharPointer(keyAddr)) + 1; param_key = new u8[keylen]; Memory::Memcpy(param_key, keyAddr, (u32)keylen, "KernelLoadParam"); } diff --git a/Core/MemMap.h b/Core/MemMap.h index b9f01fb506..c53a75f4a4 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -310,6 +310,10 @@ inline u32 MaxSizeAtAddress(const u32 address){ } } +inline const char *GetCharPointerUnchecked(const u32 address) { + return (const char *)GetPointerUnchecked(address); +} + // NOTE: Unlike the similar IsValidRange/IsValidAddress functions, this one is linear cost vs the size of the string, // for hopefully-obvious reasons. inline bool IsValidNullTerminatedString(const u32 address) { @@ -337,14 +341,11 @@ inline bool IsValidRange(const u32 address, const u32 size) { return ValidSize(address, size) == size; } -inline const char *GetCharPointerUnchecked(const u32 address) { - return (const char *)GetPointerUnchecked(address); -} - // Used for auto-converted char * parameters, which can sometimes legitimately be null - -// so we don't want to get caught in GetPointer's crash reporting. +// so we don't want to get caught in GetPointer's crash reporting +// TODO: This should use IsValidNullTerminatedString, but may be expensive since this is used so much - needs evaluation. inline const char *GetCharPointer(const u32 address) { - if (address && IsValidNullTerminatedString(address)) { + if (address && IsValidAddress(address)) { return GetCharPointerUnchecked(address); } else { return nullptr;