Skip to content

Fix XNNPACK FlatBuffer verification and header bounds checking #18784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lucylq wants to merge 1 commit into
base: main
Choose a base branch
from
+45 −0
Open

Fix XNNPACK FlatBuffer verification and header bounds checking #18784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions backends/xnnpack/runtime/XNNCompiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1816,16 +1816,19 @@ ET_NODISCARD Error XNNCompiler::compileModel(
Result<XNNHeader> header = XNNHeader::Parse(buffer_pointer, num_bytes);
const uint8_t* flatbuffer_data = nullptr;
const uint8_t* constant_data = nullptr;
size_t flatbuffer_size = 0;
CompileAllocator compile_allocator;

// Header status can only either be Error::Ok or Error::NotFound
if (header.ok()) {
flatbuffer_data = reinterpret_cast<const uint8_t*>(buffer_pointer) +
header->flatbuffer_offset;
flatbuffer_size = header->flatbuffer_size;
constant_data = reinterpret_cast<const uint8_t*>(buffer_pointer) +
header->constant_data_offset;
} else if (header.error() == Error::NotFound) {
flatbuffer_data = reinterpret_cast<const uint8_t*>(buffer_pointer);
flatbuffer_size = num_bytes;
} else {
ET_LOG(Error, "XNNHeader may be corrupt");
return header.error();
Expand All @@ -1843,6 +1846,15 @@ ET_NODISCARD Error XNNCompiler::compileModel(
"XNNPACK Delegate Serialization Format version identifier '%.4s' != expected XN00 or XN01'",
flatbuffers::GetBufferIdentifier(flatbuffer_data));
Comment on lines 1846 to 1847
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flatbuffers::GetBufferIdentifier(flatbuffer_data) is used before any minimum-length/verification check. If flatbuffer_size is < 8 (e.g., truncated/corrupt input), this can read past the provided buffer. Add an explicit minimum-size check before calling it (at least sizeof(flatbuffers::uoffset_t) + flatbuffers::kFileIdentifierLength), or perform verification/bounds checking prior to identifier access.

Copilot uses AI. Check for mistakes.

// Verify the FlatBuffer data integrity before accessing it. Without this,
// malformed data could cause out-of-bounds reads when traversing the
// FlatBuffer's internal offset tables.
flatbuffers::Verifier verifier(flatbuffer_data, flatbuffer_size);
ET_CHECK_OR_RETURN_ERROR(
verifier.VerifyBuffer<fb_xnnpack::XNNGraph>(nullptr),
DelegateInvalidCompatibility,
"FlatBuffer verification failed; data may be truncated or corrupt");
Comment on lines +1849 to +1856
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flatbuffers::Verifier is run via fb_xnnpack::VerifyXNNGraphBuffer(verifier), but the generated verifier typically enforces the schema’s file_identifier. In this repo, the runtime schema uses file_identifier "XN00" while the Python serializer produces buffers with identifier XN01 (and this function already intends to support both). As written, verification will likely reject valid XN01 payloads and break backward compatibility.

Consider verifying the buffer without enforcing the identifier (e.g., verifier.VerifyBuffer<fb_xnnpack::XNNGraph>(nullptr)), or selecting the expected identifier string ("XN00"/"XN01") based on the earlier identifier check and passing it to VerifyBuffer<...>(expected_id) instead of calling the generated VerifyXNNGraphBuffer.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@GregoryComer GregoryComer Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double check BC with this change? We don't have good CI coverage for this currently (I need to fix this).

Copy link
Copy Markdown
Contributor Author

@lucylq lucylq Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is updated to use verifier.VerifyBuffer.. which doesn't specify the magic number


auto flatbuffer_graph = fb_xnnpack::GetXNNGraph(flatbuffer_data);
// initialize xnnpack
xnn_status status = xnn_initialize(/*allocator =*/nullptr);
Expand Down
33 changes: 33 additions & 0 deletions backends/xnnpack/runtime/XNNHeader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <executorch/backends/xnnpack/runtime/XNNHeader.h>

#include <cinttypes>
#include <cstring>

#include <executorch/runtime/core/error.h>
Expand Down Expand Up @@ -64,6 +65,38 @@ Result<XNNHeader> XNNHeader::Parse(const void* data, size_t size) {
uint64_t constant_data_size =
GetUInt64LE(header_data + XNNHeader::kConstantDataSizeOffset);

// Validate that flatbuffer region does not overflow or exceed the buffer.
ET_CHECK_OR_RETURN_ERROR(
flatbuffer_offset <= size && flatbuffer_size <= size - flatbuffer_offset,
InvalidArgument,
"flatbuffer_offset: %" PRIu32 " and flatbuffer_size: %" PRIu32
" are invalid for buffer of size: %zu",
flatbuffer_offset,
flatbuffer_size,
size);
Comment on lines +69 to +76
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new error messages print uint32_t fields using %u. In this codebase there are already places using PRIu32 for uint32_t formatting; using the PRIu32 macros here as well avoids type/format mismatches on platforms where uint32_t isn’t unsigned int and keeps formatting consistent.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@GregoryComer GregoryComer Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid nit.

Copy link
Copy Markdown
Contributor Author

@lucylq lucylq Apr 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, updated!

Comment on lines +68 to +76
// Validate that constant data region does not overflow or exceed the buffer.
ET_CHECK_OR_RETURN_ERROR(
constant_data_offset <= size &&
constant_data_size <= size - constant_data_offset,
InvalidArgument,
"constant_data_offset: %" PRIu32 " and constant_data_size: %" PRIu64
" are invalid for buffer of size: %zu",
constant_data_offset,
constant_data_size,
size);

// Validate that constant data region does not overlap with flatbuffer region.
// flatbuffer should come before constant data.
ET_CHECK_OR_RETURN_ERROR(
constant_data_offset >= flatbuffer_offset &&
constant_data_offset - flatbuffer_offset >= flatbuffer_size,
InvalidArgument,
"constant_data_offset: %" PRIu32 " and flatbuffer_offset: %" PRIu32
" with flatbuffer_size: %" PRIu32 " are overlapping.",
constant_data_offset,
flatbuffer_offset,
flatbuffer_size);

return XNNHeader{
flatbuffer_offset,
flatbuffer_size,
Expand Down
Loading