Skip to content

Commit e831b3e

Browse files
authored
Bounds-check the FlatTensor root table offset before reading it (pytorch#22118)
### Summary `FlatTensorDataMap::load()` reads the flatbuffer root table without first bounds-checking the root offset: ```cpp const flat_tensor_flatbuffer::FlatTensor* flat_tensor = flat_tensor_flatbuffer::GetFlatTensor(flat_tensor_data->data()); // then immediately: flat_tensor->named_data(); // walks the root vtable flat_tensor->segments(); ``` `GetFlatTensor()` interprets the `uoffset_t` at the start of the buffer as the root-table offset and returns `buf + offset`. The first field access then walks that table's vtable. Nothing on this path runs full flatbuffer verification, so a corrupt or truncated PTD file with a bad root offset makes these accessors dereference memory **outside the buffer** (an OOB read, flagged by ASan). The program loader already guards against exactly this in `runtime/executor/program.cpp`. This change mirrors that guard for PTD files. ### What changed After the existing identifier and alignment checks, before `GetFlatTensor()`: - Confirm the buffer is at least a flatbuffer header long (`uoffset_t` + file identifier). - Confirm the root offset is `>= kMinBufferSize` and leaves room for a vtable `soffset_t` within the buffer. - Return `InvalidExternalData` otherwise (the same error the surrounding checks use). FlatTensor is **not** size-prefixed — both writers finish the buffer plain (`builder.Finish` in C++, default `flatc` in Python) and the reader uses `GetFlatTensor` (not the size-prefixed variant) — so the root offset is at byte 0, exactly as in the program case. ### Scope This is a pre-existing hardening, independent of the schema-version work in pytorch#22114 (the version gate this protects is additive; the OOB path exists today via `named_data()`/`segments()`). ### Test plan New `FlatTensorDataMapTest.RejectsOutOfBoundsRootOffset`: copies a valid PTD into a max-aligned buffer, overwrites only the root offset with a value past the end (leaving identifier + alignment intact), and asserts `load()` returns `InvalidExternalData`. Without the check, that same input is an out-of-bounds vtable read under ASan. > Note: I was unable to build/run the C++ suite in my local environment, so I'm relying on this PR's CI (which builds the runtime and runs `extension/flat_tensor` tests, including under ASan) as the authoritative check. The change mirrors an existing, tested guard in `program.cpp`.
1 parent 6a315a4 commit e831b3e

2 files changed

Lines changed: 69 additions & 0 deletions

File tree

‎extension/flat_tensor/flat_tensor_data_map.cpp‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,38 @@ ET_NODISCARD Result<const char*> FlatTensorDataMap::get_key(
289289
flat_tensor_data->data(),
290290
kMinimumAlignment);
291291

292+
// Verify that the root table offset is within bounds before dereferencing it.
293+
// GetFlatTensor() below reads the root offset from the first bytes of the
294+
// buffer and returns a pointer at buf + root_offset; reading any field then
295+
// walks that table's vtable. Nothing here runs full flatbuffer verification,
296+
// so a corrupt offset would otherwise let version()/named_data()/segments()
297+
// dereference memory outside the buffer.
298+
//
299+
// Minimum size: root offset (uoffset_t) + file identifier, i.e. the
300+
// flatbuffer header. The identifier check above already implies at least this
301+
// many bytes, but check explicitly so the bound below cannot underflow.
302+
constexpr size_t kMinBufferSize =
303+
sizeof(flatbuffers::uoffset_t) + flatbuffers::kFileIdentifierLength;
304+
ET_CHECK_OR_RETURN_ERROR(
305+
flat_tensor_data->size() >= kMinBufferSize,
306+
InvalidExternalData,
307+
"FlatTensor data size %zu is too small (minimum %zu)",
308+
flat_tensor_data->size(),
309+
kMinBufferSize);
310+
uint32_t root_offset =
311+
flatbuffers::ReadScalar<flatbuffers::uoffset_t>(flat_tensor_data->data());
312+
// The root table is at buf + root_offset. It must not point into the header
313+
// and must leave room for at least a vtable offset (soffset_t) at its
314+
// position.
315+
ET_CHECK_OR_RETURN_ERROR(
316+
root_offset >= kMinBufferSize &&
317+
root_offset <=
318+
flat_tensor_data->size() - sizeof(flatbuffers::soffset_t),
319+
InvalidExternalData,
320+
"FlatTensor root table offset %u is invalid for data size %zu",
321+
root_offset,
322+
flat_tensor_data->size());
323+
292324
// Get pointer to root of flatbuffer table.
293325
const flat_tensor_flatbuffer::FlatTensor* flat_tensor =
294326
flat_tensor_flatbuffer::GetFlatTensor(flat_tensor_data->data());

‎extension/flat_tensor/test/flat_tensor_data_map_test.cpp‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,3 +266,40 @@ TEST_F(FlatTensorDataMapTest, NewerSchemaVersionFailsToLoad) {
266266

267267
EXPECT_EQ(data_map.error(), Error::InvalidExternalData);
268268
}
269+
270+
TEST_F(FlatTensorDataMapTest, RejectsOutOfBoundsRootOffset) {
271+
// A valid file loads.
272+
Result<FreeableBuffer> valid = data_map_loader_->load(
273+
0,
274+
data_map_loader_->size().get(),
275+
DataLoader::SegmentInfo(DataLoader::SegmentInfo::Type::Constant));
276+
ASSERT_EQ(valid.error(), Error::Ok);
277+
278+
// Copy it into a max-aligned buffer so we can corrupt the root table offset
279+
// without tripping the earlier alignment check. std::vector<uint8_t> only
280+
// guarantees 1-byte alignment, so over-allocate and offset to an aligned
281+
// start. The first 4 bytes of a (non-size-prefixed) flatbuffer are the
282+
// uoffset_t pointing at the root table; the next 4 are the file identifier.
283+
constexpr size_t kAlignment = alignof(std::max_align_t);
284+
const size_t size = valid->size();
285+
std::unique_ptr<uint8_t[]> storage(new uint8_t[size + kAlignment]);
286+
const size_t offset =
287+
(kAlignment - (reinterpret_cast<uintptr_t>(storage.get()) % kAlignment)) %
288+
kAlignment;
289+
uint8_t* corrupt = storage.get() + offset;
290+
std::memcpy(corrupt, valid->data(), size);
291+
292+
// Overwrite only the root offset with a value far past the end of the buffer,
293+
// leaving the identifier and alignment intact so the corruption is caught by
294+
// the root-offset bounds check rather than the identifier or size checks.
295+
const uint32_t bad_offset = static_cast<uint32_t>(size) + 0x1000u;
296+
std::memcpy(corrupt, &bad_offset, sizeof(bad_offset));
297+
298+
// Without the bounds check, GetFlatTensor() would return a pointer outside
299+
// the buffer and the first field read would walk an out-of-bounds vtable
300+
// (a use that ASan flags). With it, load() returns cleanly.
301+
BufferDataLoader corrupt_loader(corrupt, size);
302+
Result<FlatTensorDataMap> corrupt_map =
303+
FlatTensorDataMap::load(&corrupt_loader);
304+
ASSERT_EQ(corrupt_map.error(), Error::InvalidExternalData);
305+
}

0 commit comments

Comments
 (0)