mirror of
https://github.com/hrydgard/ppsspp.git
synced 2026-08-31 09:45:24 +02:00
Fix OOB read/write via negative seek wrap in VFSFileSystem
SeekFile stored a signed s32 position into the unsigned size_t seekPos, so a negative position (e.g. from a truncating s32 cast of a large lseek offset) wrapped seekPos to near 2^64. ReadFile's clamp arithmetic then also wrapped, driving a memcpy from a wild pointer. - Clamp the computed seek position to 0 in SeekFile. - Clamp the read size against the remaining data in ReadFile, returning 0 when seekPos is at or past the end.
This commit is contained in:
@@ -1170,12 +1170,18 @@ size_t VFSFileSystem::ReadFile(u32 handle, u8 *pointer, s64 size, int &usec) {
|
||||
EntryMap::iterator iter = entries.find(handle);
|
||||
if (iter != entries.end())
|
||||
{
|
||||
if(iter->second.seekPos + size > iter->second.size)
|
||||
size = iter->second.size - iter->second.seekPos;
|
||||
if(size < 0) size = 0;
|
||||
size_t bytesRead = size;
|
||||
memcpy(pointer, iter->second.fileData + iter->second.seekPos, size);
|
||||
iter->second.seekPos += size;
|
||||
// Guard against a seekPos at or past the end of the file. Without
|
||||
// this, a wrapped seekPos (near 2^64) makes the clamp arithmetic
|
||||
// below wrap too, leading to an out-of-bounds memcpy.
|
||||
if (iter->second.seekPos >= iter->second.size)
|
||||
return 0;
|
||||
|
||||
size_t bytesRead = (size_t)size;
|
||||
size_t remaining = iter->second.size - iter->second.seekPos;
|
||||
if (bytesRead > remaining)
|
||||
bytesRead = remaining;
|
||||
memcpy(pointer, iter->second.fileData + iter->second.seekPos, bytesRead);
|
||||
iter->second.seekPos += bytesRead;
|
||||
return bytesRead;
|
||||
} else {
|
||||
ERROR_LOG(Log::FileSystem,"Cannot read file that hasn't been opened: %08x", handle);
|
||||
@@ -1196,11 +1202,17 @@ size_t VFSFileSystem::WriteFile(u32 handle, const u8 *pointer, s64 size, int &us
|
||||
size_t VFSFileSystem::SeekFile(u32 handle, s32 position, FileMove type) {
|
||||
EntryMap::iterator iter = entries.find(handle);
|
||||
if (iter != entries.end()) {
|
||||
s64 newPos = 0;
|
||||
switch (type) {
|
||||
case FILEMOVE_BEGIN: iter->second.seekPos = position; break;
|
||||
case FILEMOVE_CURRENT: iter->second.seekPos += position; break;
|
||||
case FILEMOVE_END: iter->second.seekPos = iter->second.size + position; break;
|
||||
case FILEMOVE_BEGIN: newPos = position; break;
|
||||
case FILEMOVE_CURRENT: newPos = (s64)iter->second.seekPos + position; break;
|
||||
case FILEMOVE_END: newPos = (s64)iter->second.size + position; break;
|
||||
}
|
||||
// Clamp to 0 so a negative position can't wrap the unsigned seekPos
|
||||
// to a huge value (which would then read out of bounds).
|
||||
if (newPos < 0)
|
||||
newPos = 0;
|
||||
iter->second.seekPos = (size_t)newPos;
|
||||
return iter->second.seekPos;
|
||||
} else {
|
||||
//This shouldn't happen...
|
||||
|
||||
Reference in New Issue
Block a user