From 49cd9c76d97e41548231fde0747e0fb682d7f542 Mon Sep 17 00:00:00 2001 From: Katharine Chui Date: Wed, 1 May 2024 18:40:06 +0800 Subject: [PATCH 1/2] sysclib_sprintf psp memory bound checking instead of length limits --- Core/HLE/sceKernelInterrupt.cpp | 59 ++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 23 deletions(-) diff --git a/Core/HLE/sceKernelInterrupt.cpp b/Core/HLE/sceKernelInterrupt.cpp index 72596e6ceb..49ab0ad591 100644 --- a/Core/HLE/sceKernelInterrupt.cpp +++ b/Core/HLE/sceKernelInterrupt.cpp @@ -743,9 +743,20 @@ static int sysclib_memcmp(u32 dst, u32 src, u32 size) { static int sysclib_sprintf(u32 dst, u32 fmt) { ERROR_LOG(SCEKERNEL, "Untested sysclib_sprintf(dst=%08x, fmt=%08x)", dst, fmt); - if (!Memory::IsValidAddress(dst) || !Memory::IsValidAddress(fmt)) { - // What to do? Crash, probably. - return 0; + + // check fmt before using it + int i = 0; + while (true) { + u32 char_at = fmt + i; + if (!Memory::IsValidAddress(char_at)) { + ERROR_LOG(SCEKERNEL, "sysclib_sprintf bad fmt"); + return 0; + } + const char *c = Memory::GetCharPointerUnchecked(char_at); + if (*c == '\0') { + break; + } + i++; } DEBUG_LOG(SCEKERNEL, "sysclib_sprintf fmt: %s", Memory::GetCharPointerUnchecked(fmt)); @@ -764,16 +775,8 @@ static int sysclib_sprintf(u32 dst, u32 fmt) { std::string specifier = ""; int bytes_to_read = 0; int arg_idx = 0; - int fmt_len = 0; std::string result = ""; for (const char *c = Memory::GetCharPointerUnchecked(fmt); *c != '\0'; c++) { - // in case we have a bad fmt string, try not to crash the whole emulator - fmt_len++; - if (fmt_len == 1024) { - ERROR_LOG(SCEKERNEL, "sysclib_sprintf fmt is longer than 1024"); - return 0; - } - if (!processing_specifier) { if (*c == '%') { specifier = "%"; @@ -815,17 +818,22 @@ static int sysclib_sprintf(u32 dst, u32 fmt) { } arg_idx++; - if (!Memory::IsValidAddress(val)) { - ERROR_LOG(SCEKERNEL, "sysclib_sprintf bad string reference %08x", val); - return 0; + // check and copy the string reference + i = 0; + while (true) { + u32 char_at = val + i; + if (!Memory::IsValidAddress(char_at)) { + ERROR_LOG(SCEKERNEL, "sysclib_sprintf bad string reference at %08x", val); + return 0; + } + const char* ref_c = Memory::GetCharPointerUnchecked(char_at); + if (*ref_c == '\0') { + break; + } + result.append(1, *ref_c); + i++; } - const char *str = Memory::GetCharPointerUnchecked(val); - // limit the string length and hope that we don't crash on a bad string reference - char buf[1024] = {0}; - strncpy(buf, str, sizeof(buf)); - buf[sizeof(buf) - 1] = '\0'; - result.append(buf); processing_specifier = false; break; } @@ -873,7 +881,7 @@ static int sysclib_sprintf(u32 dst, u32 fmt) { bytes_to_read = bytes_to_read - 4; read_cnt++; } - char buf[1024] = {0}; + char buf[128] = {0}; snprintf(buf, sizeof(buf), specifier.c_str(), val); buf[sizeof(buf) - 1] = '\0'; result.append(buf); @@ -895,8 +903,13 @@ static int sysclib_sprintf(u32 dst, u32 fmt) { } } - // if a small buffer was allocated by the program, we will likely crash - strcpy((char *)Memory::GetPointerUnchecked(dst), result.c_str()); + DEBUG_LOG(SCEKERNEL, "sysclib_sprintf result string has length %u, content:", result.length()); + DEBUG_LOG(SCEKERNEL, "%s", result.c_str()); + if (!Memory::IsValidRange(dst, result.length() + 1)) { + ERROR_LOG(SCEKERNEL, "sysclib_sprintf result string is too long or dst is invalid"); + return 0; + } + memcpy((char *)Memory::GetPointerUnchecked(dst), result.c_str(), result.length() + 1); return result.length(); } From b570cacd90e9063d6635af0f8ad66e14a9820c13 Mon Sep 17 00:00:00 2001 From: Katharine Chui Date: Wed, 1 May 2024 22:45:32 +0800 Subject: [PATCH 2/2] add Memory::IsValidNullTerminatedString --- Core/HLE/sceKernelInterrupt.cpp | 35 +++++++-------------------------- Core/MemMap.h | 15 ++++++++++++++ 2 files changed, 22 insertions(+), 28 deletions(-) diff --git a/Core/HLE/sceKernelInterrupt.cpp b/Core/HLE/sceKernelInterrupt.cpp index 49ab0ad591..ab3fc4ff7b 100644 --- a/Core/HLE/sceKernelInterrupt.cpp +++ b/Core/HLE/sceKernelInterrupt.cpp @@ -744,19 +744,9 @@ static int sysclib_memcmp(u32 dst, u32 src, u32 size) { static int sysclib_sprintf(u32 dst, u32 fmt) { ERROR_LOG(SCEKERNEL, "Untested sysclib_sprintf(dst=%08x, fmt=%08x)", dst, fmt); - // check fmt before using it - int i = 0; - while (true) { - u32 char_at = fmt + i; - if (!Memory::IsValidAddress(char_at)) { - ERROR_LOG(SCEKERNEL, "sysclib_sprintf bad fmt"); - return 0; - } - const char *c = Memory::GetCharPointerUnchecked(char_at); - if (*c == '\0') { - break; - } - i++; + if (!Memory::IsValidNullTerminatedString(fmt)) { + ERROR_LOG(SCEKERNEL, "sysclib_sprintf bad fmt"); + return 0; } DEBUG_LOG(SCEKERNEL, "sysclib_sprintf fmt: %s", Memory::GetCharPointerUnchecked(fmt)); @@ -818,22 +808,11 @@ static int sysclib_sprintf(u32 dst, u32 fmt) { } arg_idx++; - // check and copy the string reference - i = 0; - while (true) { - u32 char_at = val + i; - if (!Memory::IsValidAddress(char_at)) { - ERROR_LOG(SCEKERNEL, "sysclib_sprintf bad string reference at %08x", val); - return 0; - } - const char* ref_c = Memory::GetCharPointerUnchecked(char_at); - if (*ref_c == '\0') { - break; - } - result.append(1, *ref_c); - i++; + if (!Memory::IsValidNullTerminatedString(val)) { + ERROR_LOG(SCEKERNEL, "sysclib_sprintf bad string reference at %08x", val); + return 0; } - + result.append(Memory::GetCharPointerUnchecked(val)); processing_specifier = false; break; } diff --git a/Core/MemMap.h b/Core/MemMap.h index 67e17babcd..8a8b62d89e 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -310,6 +310,21 @@ inline bool IsValidAddress(const u32 address) { } } +inline bool IsValidNullTerminatedString(const u32 address) { + int i = 0; + while (true) { + u32 char_at = address + i; + if (!IsValidAddress(char_at)) { + return false; + } + const char *c = GetCharPointerUnchecked(char_at); + if (*c == '\0') { + return true; + } + i++; + } +} + inline u32 ValidSize(const u32 address, const u32 requested_size) { u32 max_size; if ((address & 0x3E000000) == 0x08000000) {