Merge pull request #22118 from hrydgard/claude-elf-fixes

Claude code review: ElfReader
2026-09-04 19:55:27 +02:00 · 2026-08-19 21:22:56 +02:00
parent b00e239e41 4c72c13a0d
commit e52fd6d0ef
8 changed files with 247 additions and 61 deletions
@@ -15,10 +15,7 @@
 // Official git repository and contact information can be found at
 // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/.

-#include <atomic>
-
 #include "Common/StringUtils.h"
-#include "Common/Thread/ParallelLoop.h"
 #include "Common/File/DirListing.h"
 #include "Common/File/FileUtil.h"

@@ -56,7 +53,9 @@ void addrToHiLo(u32 addr, u16 &hi, s16 &lo)
 	lo = (addr & 0xFFFF);
 	u32 naddr = addr - lo;
 	hi = naddr>>16;
-	u32 test = (hi<<16) + lo;
+	// Note the casts: hi is a u16, so it promotes to int, and kernel modules load at 0x88000000 -
+	// shifting a value of 0x8800 left by 16 would overflow a signed int.
+	u32 test = ((u32)hi << 16) + (u32)lo;
 	if (test != addr)
 	{
 		WARN_LOG_REPORT(Log::Loader, "HI16/LO16 relocation failure?");
@@ -68,8 +67,7 @@ bool ElfReader::LoadRelocations(const Elf32_Rel *rels, int numRelocs) {
 	relocOps.resize(numRelocs);

 	DEBUG_LOG(Log::Loader, "Loading %i relocations...", numRelocs);
-	std::atomic<int> numErrors;
-	numErrors.store(0);
+	int numErrors = 0;

 	{
 		for (int r = 0; r < numRelocs; r++) {
@@ -161,7 +159,12 @@ bool ElfReader::LoadRelocations(const Elf32_Rel *rels, int numRelocs) {
 					if (t_type == R_MIPS_HI16)
 						continue;

-					u32 corrLoAddr = rels[t].r_offset + segmentVAddr[readwrite];
+					// The candidate LO16 declares its own segment - use that rather than the HI16's,
+					// which is what the mismatch warning further down is there to detect.
+					int t_readwrite = (rels[t].r_info >> 8) & 0xff;
+					if (t_readwrite >= (int)ARRAY_SIZE(segmentVAddr))
+						continue;
+					u32 corrLoAddr = rels[t].r_offset + segmentVAddr[t_readwrite];

 					// In MotorStorm: Arctic Edge (US), these are sometimes R_MIPS_16 (instead of LO16.)
 					// It appears the PSP takes any relocation that is not a HI16.
@@ -194,10 +197,14 @@ bool ElfReader::LoadRelocations(const Elf32_Rel *rels, int numRelocs) {
 						ERROR_LOG(Log::Loader, "Bad corrLoAddr %08x", corrLoAddr);
 					}
 				}
-				if (!found) {
+				if (found) {
+					op = (op & 0xFFFF0000) | hi;
+				} else {
+					// Leave the instruction alone rather than writing hi's initial 0 into it. We
+					// have no idea what the right immediate is, and zeroing the lui of a lui/addiu
+					// pair is a guess that's wrong in a way that's hard to trace back to here.
 					ERROR_LOG_REPORT(Log::Loader, "R_MIPS_HI16: could not find R_MIPS_LO16 (r=%d of %d, addr=%08x)", r, numRelocs, addr);
 				}
-				op = (op & 0xFFFF0000) | hi;
 			}
 			break;

@@ -239,7 +246,7 @@ bool ElfReader::LoadRelocations(const Elf32_Rel *rels, int numRelocs) {
 	}

 	if (numErrors) {
-		WARN_LOG(Log::Loader, "%i bad relocations found!!!", numErrors.load());
+		WARN_LOG(Log::Loader, "%i bad relocations found!!!", numErrors);
 	}
 	return numErrors == 0;
 }
@@ -263,8 +270,26 @@ void ElfReader::LoadRelocations2(int rel_seg)
 		ERROR_LOG_REPORT(Log::Loader, "Rel2 segment invalid");
 		return;
 	}
+	// GetSegmentPtr only vouches for where the segment starts - p_filesz comes from the file too.
+	if ((size_t)ph->p_offset + ph->p_filesz > size_) {
+		ERROR_LOG_REPORT(Log::Loader, "Rel2 segment extends past the end of the file");
+		return;
+	}
 	end = buf+ph->p_filesz;

+	// Everything below reads forward from buf, so check there's something there each time. All of
+	// these sizes and indexes come out of the file.
+	auto haveBytes = [&buf, &end](int n) -> bool {
+		if (end - buf < n) {
+			ERROR_LOG_REPORT(Log::Loader, "Rel2: truncated relocation data");
+			return false;
+		}
+		return true;
+	};
+
+	if (!haveBytes(4))
+		return;
+
 	flag_bits = buf[2];
 	type_bits = buf[3];

@@ -274,22 +299,40 @@ void ElfReader::LoadRelocations2(int rel_seg)

 	buf += 4;

+	// Both tables are prefixed by their own size, and are indexed by bitfields out of the command
+	// words below - so the tables and the indexes into them both need checking.
+	if (!haveBytes(1))
+		return;
 	flag_table = buf;
 	flag_table_size = flag_table[0];
+	if (!haveBytes(flag_table_size))
+		return;
 	buf += flag_table_size;

+	if (!haveBytes(1))
+		return;
 	type_table = buf;
 	type_table_size = type_table[0];
+	if (!haveBytes(type_table_size))
+		return;
 	buf += type_table_size;

 	rel_base = 0;
 	last_type = -1;
 	while(buf<end){
-		cmd = *(u16*)(buf);
+		if (!haveBytes(2))
+			return;
+		// Byte-wise rather than *(u16 *)buf: how far buf has advanced depends on the table sizes
+		// above, so it isn't necessarily even.
+		cmd = buf[0] | (buf[1] << 8);
 		buf += 2;

 		flag = ( cmd<<(16-flag_bits))&0xffff;
 		flag = (flag>>(16-flag_bits))&0xffff;
+		if (flag >= flag_table_size) {
+			ERROR_LOG_REPORT(Log::Loader, "Rel2: flag %d out of range (table has %d)", flag, flag_table_size);
+			return;
+		}
 		flag = flag_table[flag];

 		seg = (cmd<<(16-seg_bits-flag_bits))&0xffff;
@@ -297,6 +340,10 @@ void ElfReader::LoadRelocations2(int rel_seg)

 		type = ( cmd<<(16-type_bits-seg_bits-flag_bits))&0xffff;
 		type = (type>>(16-type_bits))&0xffff;
+		if (type >= type_table_size) {
+			ERROR_LOG_REPORT(Log::Loader, "Rel2: type %d out of range (table has %d)", type, type_table_size);
+			return;
+		}
 		type = type_table[type];

 		if((flag&0x01)==0){
@@ -304,6 +351,8 @@ void ElfReader::LoadRelocations2(int rel_seg)
 			if((flag&0x06)==0){
 				rel_base = cmd>>(seg_bits+flag_bits);
 			}else if((flag&0x06)==4){
+				if (!haveBytes(4))
+					return;
 				rel_base = buf[0] | (buf[1]<<8) | (buf[2]<<16) | (buf[3]<<24);
 				buf += 4;
 			}else{
@@ -333,17 +382,25 @@ void ElfReader::LoadRelocations2(int rel_seg)
 				if(cmd&0x8000)
 					rel_offset |= 0xffff0000;
 				rel_offset >>= type_bits+seg_bits+flag_bits;
+				if (!haveBytes(2))
+					return;
 				rel_offset = (rel_offset<<16) | (buf[0]) | (buf[1]<<8);
 				buf += 2;
 				rel_base += rel_offset;
 			}else if((flag&0x06)==0x04){
+				if (!haveBytes(4))
+					return;
 				rel_base = buf[0] | (buf[1]<<8) | (buf[2]<<16) | (buf[3]<<24);
 				buf += 4;
 			}else{
 				ERROR_LOG_REPORT(Log::Loader, "Rel2: invalid relocat size flag! %x", flag);
 			}

-
+			// seg is seg_bits wide, which can address more segments than we can record.
+			if (off_seg >= (int)ARRAY_SIZE(segmentVAddr)) {
+				ERROR_LOG_REPORT(Log::Loader, "Rel2: bad offset segment %d", off_seg);
+				continue;
+			}
 			rel_offset = rel_base+segmentVAddr[off_seg];
 			if (!Memory::IsValidAddress(rel_offset)) {
 				ERROR_LOG_REPORT(Log::Loader, "ELF: Bad rel_offset: %08x", rel_offset);
@@ -356,6 +413,8 @@ void ElfReader::LoadRelocations2(int rel_seg)
 				if(last_type!=0x04)
 					lo16 = 0;
 			}else if((flag&0x38)==0x10){
+				if (!haveBytes(2))
+					return;
 				lo16 = (buf[0]) | (buf[1]<<8);
 				if(lo16&0x8000)
 					lo16 |= 0xffff0000;
@@ -438,6 +497,14 @@ int ElfReader::LoadInto(u32 loadAddress, bool fromTop) {
 		return SCE_KERNEL_ERROR_MEMBLOCK_ALLOC_FAILED;
 	}

+	// e_phnum is a u16, but we can only record the load address of ARRAY_SIZE(segmentVAddr) segments,
+	// and the relocation code can't refer to segments beyond that either (see LoadRelocations). Real
+	// PSP modules have a handful - PSP_Header::nsegments is a u8 and no more than 4 are ever used.
+	if (GetNumSegments() > (int)ARRAY_SIZE(segmentVAddr)) {
+		ERROR_LOG(Log::Loader, "ELF has %d segments, we support at most %d", GetNumSegments(), (int)ARRAY_SIZE(segmentVAddr));
+		return SCE_KERNEL_ERROR_MEMBLOCK_ALLOC_FAILED;
+	}
+
 	// e_ident[EI_VERSION] is ignored

 	// Should we relocate?
@@ -467,9 +534,11 @@ int ElfReader::LoadInto(u32 loadAddress, bool fromTop) {
 	entryPoint = header->e_entry;
 	u32 totalStart = 0xFFFFFFFF;
 	u32 totalEnd = 0;
+	int numLoadSegments = 0;
 	for (int i = 0; i < header->e_phnum; i++) {
 		const Elf32_Phdr *p = &segments[i];
 		if (p->p_type == PT_LOAD) {
+			numLoadSegments++;
 			if (p->p_vaddr < totalStart) {
 				totalStart = p->p_vaddr;
 				firstSegAlign = p->p_align;
@@ -478,6 +547,12 @@ int ElfReader::LoadInto(u32 loadAddress, bool fromTop) {
 				totalEnd = p->p_vaddr + p->p_memsz;
 		}
 	}
+	// Without this, totalStart stays 0xFFFFFFFF and totalEnd 0, so totalSize would come out as 1
+	// and we'd go on to allocate at 0xFFFFFFFF.
+	if (numLoadSegments == 0) {
+		ERROR_LOG(Log::Loader, "ELF has no loadable segments");
+		return SCE_KERNEL_ERROR_MEMBLOCK_ALLOC_FAILED;
+	}
 	totalSize = totalEnd - totalStart;

 	// If a load address is specified that's in regular RAM, override kernel module status
@@ -628,7 +703,7 @@ int ElfReader::LoadInto(u32 loadAddress, bool fromTop) {
 		{
 			//We have a relocation table!
 			int sectionToModify = s->sh_info;
-			if (sectionToModify >= 0)
+			if (sectionToModify >= 0 && sectionToModify < GetNumSections())
 			{
 				if (!(sections[sectionToModify].sh_flags & SHF_ALLOC))
 				{
@@ -663,7 +738,7 @@ int ElfReader::LoadInto(u32 loadAddress, bool fromTop) {
 			{
 				//We have a relocation table!
 				int sectionToModify = s->sh_info;
-				if (sectionToModify >= 0)
+				if (sectionToModify >= 0 && sectionToModify < GetNumSections())
 				{
 					if (!(sections[sectionToModify].sh_flags & SHF_ALLOC))
 					{
@@ -788,10 +863,15 @@ bool ElfReader::LoadSymbols()
 		u32 symtabOffset = GetSectionDataOffset(sec);

 		int numSymbols = sections[sec].sh_size / sizeof(Elf32_Sym);
-		if (!stringBase || !symtab || symtabOffset + sections[sec].sh_size > size_) {
+		if (!stringBase || !symtab || (size_t)symtabOffset + sections[sec].sh_size > size_) {
 			ERROR_LOG(Log::Loader, "Symbols truncated - ignoring");
 			return false;
 		}
+		// Relocating a symbol needs the section addresses LoadInto computed.
+		if (bRelocate && !sectionAddrs) {
+			ERROR_LOG(Log::Loader, "LoadSymbols called before LoadInto - ignoring");
+			return false;
+		}
 		
 		for (int sym = 0; sym<numSymbols; sym++)
 		{
@@ -803,12 +883,29 @@ bool ElfReader::LoadSymbols()
 			int type = symtab[sym].st_info & 0xF;
 			int sectionIndex = symtab[sym].st_shndx;
 			int value = symtab[sym].st_value;
+			const size_t nameOffset = (size_t)stringOffset + symtab[sym].st_name;
+			if (nameOffset >= size_)
+				continue;
 			const char *name = stringBase + symtab[sym].st_name;
-			if (stringOffset + symtab[sym].st_name >= size_)
+			// And make sure it's terminated inside the file, before anything strlen()s it.
+			if (strnlen(name, size_ - nameOffset) == size_ - nameOffset)
 				continue;

-			if (bRelocate)
+			if (bRelocate) {
+				// st_shndx is a u16 that can hold reserved values rather than a section number -
+				// SHN_ABS (0xFFF1) in particular is common and means the value is already final.
+				// Indexing sectionAddrs (which has GetNumSections() entries) with one of those read
+				// far out of bounds and added whatever it found to the symbol's address.
+				if (sectionIndex == SHN_UNDEF || sectionIndex >= SHN_LORESERVE) {
+					// Undefined, absolute or common - nothing of ours to relocate against.
+					continue;
+				}
+				if (sectionIndex >= GetNumSections()) {
+					WARN_LOG(Log::Loader, "Symbol '%s' refers to bad section %d, skipping", name, sectionIndex);
+					continue;
+				}
 				value += sectionAddrs[sectionIndex];
+			}

 			switch (type)
 			{
@@ -51,10 +51,14 @@ public:
 	ElfReader(const void *ptr, size_t size) {
 		base = (const char*)ptr;
 		base32 = (const u32 *)ptr;
+		size_ = size;
+		// Don't read the header to find the segment and section tables before we know it's there.
+		// LoadInto() rejects anything this small; header stays null so nothing else can use it either.
+		if (size < sizeof(Elf32_Ehdr))
+			return;
 		header = (const Elf32_Ehdr*)ptr;
 		segments = (const Elf32_Phdr *)(base + header->e_phoff);
 		sections = (const Elf32_Shdr *)(base + header->e_shoff);
-		size_ = size;
 	}

 	~ElfReader() {
@@ -66,21 +70,22 @@ public:
 		return base32[off >> 2];
 	}

-	// Quick accessors
-	ElfType GetType() const { return (ElfType)(u16)(header->e_type); }
-	ElfMachine GetMachine() const { return (ElfMachine)(u16)(header->e_machine); }
+	// Quick accessors. header is null if we weren't even handed a full ELF header, see the
+	// constructor - so these all have to cope with that.
+	ElfType GetType() const { return header ? (ElfType)(u16)(header->e_type) : (ElfType)0; }
+	ElfMachine GetMachine() const { return header ? (ElfMachine)(u16)(header->e_machine) : (ElfMachine)0; }
 	u32 GetEntryPoint() const { return entryPoint; }
-	u32 GetFlags() const { return (u32)(header->e_flags); }
+	u32 GetFlags() const { return header ? (u32)(header->e_flags) : 0; }

-	int GetNumSegments() const { return (int)(header->e_phnum); }
-	int GetNumSections() const { return (int)(header->e_shnum); }
+	int GetNumSegments() const { return header ? (int)(header->e_phnum) : 0; }
+	int GetNumSections() const { return header ? (int)(header->e_shnum) : 0; }
 	const char *GetSectionName(int section) const;
 	const u8 *GetPtr(u32 offset) const {
 		return (const u8*)base + offset;
 	}
 	// Note: zero is not a valid output, means unavailable.
 	u32 GetSectionDataOffset(int section) const {
-		if (section < 0 || section >= header->e_shnum)
+		if (section < 0 || section >= GetNumSections())
 			return 0;
 		if (sections[section].sh_type == SHT_NOBITS)
 			return 0;
@@ -88,19 +93,26 @@ public:
 	}
 	const u8 *GetSectionDataPtr(int section) const {
 		u32 offset = GetSectionDataOffset(section);
-		if (offset == 0 || offset > size_)
+		// Note >=: an offset exactly at the end of the file addresses no bytes at all.
+		if (offset == 0 || offset >= size_)
 			return nullptr;
 		return GetPtr(offset);
 	}
 	const u8 *GetSegmentPtr(int segment) const {
-		if (segments[segment].p_offset > size_)
+		if (segment < 0 || segment >= GetNumSegments())
+			return nullptr;
+		if (segments[segment].p_offset >= size_)
 			return nullptr;
 		return GetPtr(segments[segment].p_offset);
 	}
 	u32 GetSectionAddr(SectionID section) const {
+		if (section < 0 || section >= GetNumSections() || !sectionAddrs)
+			return 0;
 		return sectionAddrs[section];
 	}
 	int GetSectionSize(SectionID section) const {
+		if (section < 0 || section >= GetNumSections())
+			return 0;
 		return sections[section].sh_size;
 	}

@@ -29,16 +29,20 @@ PBPReader::PBPReader(FileLoader *fileLoader) {
 	}

 	fileSize_ = (size_t)fileLoader->FileSize();
-	if (fileLoader->ReadAt(0, sizeof(header_), (u8 *)&header_) != sizeof(header_)) {
+	if (fileLoader->ReadAt(0, sizeof(header_), &header_) != sizeof(header_)) {
 		ERROR_LOG(Log::Loader, "PBP is too small to be valid: %s", fileLoader->GetPath().c_str());
 		return;
 	}
 	if (memcmp(header_.magic, "\0PBP", 4) != 0) {
-		if (memcmp(header_.magic, "\nFLE", 4) != 0) {
+		// Split string so the \x7f escape doesn't swallow the E. This used to compare against
+		// "\nFLE", which is neither ELF's magic nor anything else - so every file that wasn't a PBP
+		// was reported as an ELF, and the error branch below was unreachable.
+		if (memcmp(header_.magic, "\x7f" "ELF", 4) == 0) {
 			VERBOSE_LOG(Log::Loader, "%s: File actually an ELF, not a PBP", fileLoader->GetPath().c_str());
 			isELF_ = true;
 		} else {
-			ERROR_LOG(Log::Loader, "Magic number in %s indicated no PBP: %s", fileLoader->GetPath().c_str(), header_.magic);
+			ERROR_LOG(Log::Loader, "Magic number in %s indicates neither PBP nor ELF: %02x %02x %02x %02x",
+				fileLoader->GetPath().c_str(), (u8)header_.magic[0], (u8)header_.magic[1], (u8)header_.magic[2], (u8)header_.magic[3]);
 		}
 		return;
 	}
@@ -63,7 +67,9 @@ bool PBPReader::GetSubFile(PBPSubFile file, std::vector<u8> *out) const {
 	const u32 off = header_.offsets[(int)file];

 	out->resize(expected);
-	size_t bytes = file_->ReadAt(off, expected, &(*out)[0]);
+	if (expected == 0)
+		return true;
+	size_t bytes = file_->ReadAt(off, expected, out->data());
 	if (bytes != expected) {
 		ERROR_LOG(Log::Loader, "PBP file read truncated: %d -> %d", (int)expected, (int)bytes);
 		if (bytes < expected) {
@@ -20,6 +20,7 @@

 #include <vector>

+#include "Common/Common.h"
 #include "Common/CommonTypes.h"
 #include "Common/Swap.h"

@@ -54,17 +55,25 @@ public:
 	bool GetSubFileAsString(PBPSubFile file, std::string *out) const;

 	size_t GetSubFileSize(PBPSubFile file) const {
-		int num = (int)file;
-		if (num < 7) {
-			return header_.offsets[file + 1] - header_.offsets[file];
-		} else {
-			return fileSize_ - header_.offsets[file];
-		}
+		const int num = (int)file;
+		if (num < 0 || num >= (int)ARRAY_SIZE(header_.offsets))
+			return 0;
+		const u32 start = header_.offsets[num];
+		// The last subfile runs to the end of the file, the rest to where the next one starts.
+		const u32 stop = num + 1 < (int)ARRAY_SIZE(header_.offsets) ? header_.offsets[num + 1] : (u32)fileSize_;
+		// These offsets come out of the file, so they aren't necessarily ordered or even inside it.
+		// Subtracting them blind produced a huge size from an underflow.
+		if (stop < start || stop > fileSize_)
+			return 0;
+		return stop - start;
 	}

 private:
 	FileLoader *file_ = nullptr;
 	size_t fileSize_ = 0;
-	const PBPHeader header_{};
+	// Not const: the constructor reads the file straight into this. It used to be, and was written
+	// through a cast that stripped the const away - which compiles, but lets the compiler assume the
+	// value never changes from the {} it was initialized with.
+	PBPHeader header_{};
 	bool isELF_ = false;
 };
@@ -15,6 +15,7 @@
 // Official git repository and contact information can be found at
 // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/.

+#include <algorithm>
 #include <cstdio>
 #include <cstring>

@@ -227,14 +228,18 @@ int ParamSFOData::GetDataOffset(const u8 *paramsfo, size_t size, const char *dat
 	const IndexTable *indexTables = (const IndexTable *)(paramsfo + sizeof(Header));

 	const u8 *key_start = paramsfo + header->key_table_start;
-	int data_start = header->data_table_start;
+	const size_t data_start = header->data_table_start;

 	for (u32 i = 0; i < header->index_table_entries; i++)
 	{
+		// In size_t throughout - these are u32s from the file, and mixing them with int meant the
+		// bounds check below was done in whatever type the promotion landed on. ReadSFO does the
+		// same arithmetic this way.
 		size_t key_offset = header->key_table_start + indexTables[i].key_table_offset;
 		if (key_offset >= size)
 			continue;
-		if (data_start + indexTables[i].data_table_offset >= (int)size)
+		size_t data_offset = data_start + indexTables[i].data_table_offset;
+		if (data_offset >= size)
 			continue;

 		const char *key = (const char *)(key_start + indexTables[i].key_table_offset);
@@ -243,13 +248,19 @@ int ParamSFOData::GetDataOffset(const u8 *paramsfo, size_t size, const char *dat
 			continue;
 		if (!strcmp(key, dataName))
 		{
-			return data_start + indexTables[i].data_table_offset;
+			return (int)data_offset;
 		}
 	}

 	return -1;
 }

+// How many bytes an entry gets in the data table. Everything written for it has to fit in here -
+// the size loop below and the fill loop after it both go through this, so they can't disagree.
+static u32 ReservedSize(const ParamSFOData::ValueData &value) {
+	return (u32)std::max(0, value.max_size);
+}
+
 void ParamSFOData::WriteSFO(u8 **paramsfo, size_t *size) const {
 	size_t total_size = 0;
 	size_t key_size = 0;
@@ -266,7 +277,7 @@ void ParamSFOData::WriteSFO(u8 **paramsfo, size_t *size) const {
 	for (const auto &[k, v] : values)
 	{
 		key_size += k.size() + 1;
-		data_size += v.max_size;
+		data_size += ReservedSize(v);

 		header.index_table_entries++;
 	}
@@ -299,35 +310,49 @@ void ParamSFOData::WriteSFO(u8 **paramsfo, size_t *size) const {
 		index_ptr->key_table_offset = offset;
 		offset = (u16)(data_ptr - (data+header.data_table_start));
 		index_ptr->data_table_offset = offset;
-		index_ptr->param_max_len = v.max_size;
+		const u32 reserved = ReservedSize(v);
+		index_ptr->param_max_len = reserved;
 		if (v.type == VT_INT)
 		{
 			index_ptr->param_fmt = 0x0404;
-			index_ptr->param_len = 4;
+			index_ptr->param_len = std::min(4u, reserved);

-			*(s32_le *)data_ptr = v.i_value;
+			if (reserved >= 4)
+				*(s32_le *)data_ptr = v.i_value;
+			else
+				WARN_LOG(Log::Loader, "SFO key '%s' is an int but only reserves %d bytes, dropping", k.c_str(), (int)reserved);
 		}
 		else if (v.type == VT_UTF8_SPE)
 		{
 			index_ptr->param_fmt = 0x0004;
-			index_ptr->param_len = (u32)v.u_value.size();
+			// Raw data, no terminator, but it still has to fit in what the entry reserved.
+			const u32 len = std::min((u32)v.u_value.size(), reserved);
+			if (len != v.u_value.size())
+				WARN_LOG(Log::Loader, "SFO key '%s': %d bytes of data truncated to %d", k.c_str(), (int)v.u_value.size(), (int)len);
+			index_ptr->param_len = len;

-			memset(data_ptr, 0, index_ptr->param_max_len);
-			memcpy(data_ptr, v.u_value.data(), index_ptr->param_len);
+			memset(data_ptr, 0, reserved);
+			memcpy(data_ptr, v.u_value.data(), len);
 		}
 		else if (v.type == VT_UTF8)
 		{
 			index_ptr->param_fmt = 0x0204;
-			index_ptr->param_len = (u32)v.s_value.size()+1;
+			// param_len counts the NUL terminator, so the string itself gets reserved - 1 bytes.
+			// Several callers pass the string's own length as max_size (see PSPLoaders.cpp), which
+			// used to overrun the entry by the terminator plus one more from the stray write below.
+			const u32 len = std::min((u32)v.s_value.size(), reserved ? reserved - 1 : 0);
+			if (len != v.s_value.size())
+				WARN_LOG(Log::Loader, "SFO key '%s': string of %d chars truncated to %d", k.c_str(), (int)v.s_value.size(), (int)len);
+			index_ptr->param_len = reserved ? len + 1 : 0;

-			memcpy(data_ptr,v.s_value.c_str(),index_ptr->param_len);
-			data_ptr[index_ptr->param_len] = 0;
+			memset(data_ptr, 0, reserved);  // Also supplies the terminator.
+			memcpy(data_ptr, v.s_value.data(), len);
 		}

 		memcpy(key_ptr,k.c_str(),k.size());
 		key_ptr[k.size()] = 0;

-		data_ptr += index_ptr->param_max_len;
+		data_ptr += reserved;
 		key_ptr += k.size() + 1;
 		index_ptr++;

@@ -348,14 +373,18 @@ std::string ParamSFOData::GenerateFakeID(const Path &filename) const {

 	std::string file = path.GetFilename();

+	// Deliberately byte-wise and ASCII-only. Filenames are UTF-8, and a plain char is signed on x86
+	// and unsigned on ARM - so summing chars directly gave Windows and Android different IDs for the
+	// same non-ASCII folder name, and toupper() on a negative value trips MSVC's debug CRT. ASCII
+	// names, which is very nearly all of them, produce exactly the same ID as before either way.
 	int sumOfAllLetters = 0;
 	for (char &c : file) {
-		sumOfAllLetters += c;
+		sumOfAllLetters += (unsigned char)c;
 		// Get rid of some garbage characters than can arise when opening content URIs. Well, I've only seen '%', but...
-		if (strchr("%() []", c) != nullptr) {
+		if (c && strchr("%() []", c) != nullptr) {
 			c = 'X';
-		} else {
-			c = toupper(c);
+		} else if (c >= 'a' && c <= 'z') {
+			c = c - 'a' + 'A';
 		}
 	}

@@ -1017,6 +1017,16 @@ static int pspDecryptType6(KirkState *kirk, const u8 *inbuf, u8 *outbuf, u32 siz

 int pspDecryptPRX(const u8 *inbuf, u8 *outbuf, u32 size, const u8 *seed)
 {
+	// Every type below reads the tag at 0xD0, the size at 0xB0 and key data as far as 0x150, and
+	// writes a KIRK header into outbuf at a fixed offset derived from sizeof(PSP_Header). Without
+	// this, a PRX declaring a tiny psp_size got read past its end, had a header written past the end
+	// of the (equally tiny) output buffer, and passed "size - offset" to KIRK as an unsigned
+	// underflow. Callers must supply at least a whole header on both sides.
+	if (size < sizeof(PSP_Header)) {
+		ERROR_LOG(Log::Loader, "PRX too small to decrypt: %d bytes, need at least %d", (int)size, (int)sizeof(PSP_Header));
+		return -1;
+	}
+
 	KirkState kirk{};
 	kirk_init(&kirk);

@@ -1663,7 +1663,7 @@ static u32 sceKernelGetMemoryBlockAddr(u32 uid, u32 addr) {
 	PartitionMemoryBlock *block = kernelObjects.Get<PartitionMemoryBlock>(uid, error);
 	if (block) {
 		Memory::WriteOrException_U32(block->address, addr);
-		return hleLogInfo(Log::sceKernel, 0, "block address: %08x", block->address);
+		return hleLogDebug(Log::sceKernel, 0, "block address: %08x", block->address);
 	} else {
 		return hleLogError(Log::sceKernel, 0, "failed");
 	}
@@ -1095,6 +1095,15 @@ enum : u32 {

 // filename is only used for dumping/metadata.
 static PSPModule *__KernelLoadELFFromPtr(const u8 *ptr, size_t elfSize, u32 loadAddress, bool fromTop, std::string *error_string, u32 *magic, std::string_view filename, u32 &error) {
+	// The magic reads below need four bytes, and the ~SCE branch another four after that. Everything
+	// downstream checks its own sizes; this is just so we can look at the magic at all. The PBP path
+	// in __KernelLoadModule computes elfSize from two offsets in the file and doesn't floor it.
+	if (elfSize < 2 * sizeof(u32)) {
+		*error_string = "ELF file truncated - can't load";
+		error = SCE_KERNEL_ERROR_FILEERR;
+		return nullptr;
+	}
+
 	PSPModule *module = new PSPModule();
 	kernelObjects.Create(module);
 	loadedModules.insert(module->GetUID());
@@ -1159,16 +1168,23 @@ static PSPModule *__KernelLoadELFFromPtr(const u8 *ptr, size_t elfSize, u32 load
 		elfSize = maxElfSize;
 		ptr = newptr;
 		int decryptedSize = pspDecryptPRX(in, (u8*)ptr, head->psp_size);
-		_dbg_assert_(decryptedSize <= (int)maxElfSize);
-		if (decryptedSize <= 0 && Read32(ptr + 0x150) == ELF_MAGIC) {
+		// If decryption got us nowhere, the PRX may simply not be encrypted - in which case the ELF
+		// starts right after the header. Check the source buffer, not the destination: on the paths
+		// where decryption bails early nothing has been written to newptr yet, so this used to read
+		// uninitialized heap to decide. psp_size is known to be <= the data we actually have.
+		if (decryptedSize <= 0 && head->psp_size >= 0x150 + sizeof(u32) && Read32(in + 0x150) == ELF_MAGIC) {
 			decryptedSize = head->psp_size - 0x150;
 			memcpy(newptr, in + 0x150, decryptedSize);
 			// In this case it's definitely not compressed. Added assert below.
 		}

-		// Don't accept ELFs over 24MB - nor ones with negative size, of course.
-		if (decryptedSize < 0 || decryptedSize > 24 * 1024 * 1024) {
+		// Don't accept ELFs over 24MB, ones bigger than the buffer we allocated for them - nor ones
+		// with negative size, of course.
+		if (decryptedSize < 0 || decryptedSize > 24 * 1024 * 1024 || decryptedSize > (int)maxElfSize) {
 			*error_string = StringFromFormat("ELF/PRX corrupt, unreasonable decrypted size: %d", (u32)decryptedSize);
+			delete [] newptr;
+			module->Cleanup();
+			kernelObjects.Destroy<PSPModule>(module->GetUID());
 			// TODO: Might be the wrong error code.
 			error = SCE_KERNEL_ERROR_FILEERR;
 			return nullptr;
@@ -1190,6 +1206,9 @@ static PSPModule *__KernelLoadELFFromPtr(const u8 *ptr, size_t elfSize, u32 load
 				// Bail out cleanly here rather than falling through to parse whatever's left
 				// in the buffer (still compressed, not a valid ELF) as if it were real code.
 				*error_string = StringFromFormat("Module '%s' decompression failed", head->modname);
+				delete [] newptr;
+				module->Cleanup();
+				kernelObjects.Destroy<PSPModule>(module->GetUID());
 				// TODO: Might be the wrong error code.
 				error = SCE_KERNEL_ERROR_FILEERR;
 				return nullptr;
@@ -1210,6 +1229,10 @@ static PSPModule *__KernelLoadELFFromPtr(const u8 *ptr, size_t elfSize, u32 load
 			// This should happen for all "kernel" modules.
 			*error_string = "Missing key";
 			delete [] newptr;
+			// ptr still points into this buffer, but nothing below reads it - and the exits further
+			// down all free newptr, so it has to be null by the time they're reached.
+			newptr = nullptr;
+			ptr = nullptr;
 			module->isFake = true;
 			strncpy(module->nm.name, head->modname, ARRAY_SIZE(module->nm.name));
 			module->nm.entry_addr = -1;