Allowlist safe LoRA adapter parameter data types - #31682
Conversation
CreateOrtValueOverLoraParameter now rejects TensorDataType::STRING in addition to UNDEFINED. The flatbuffer schema includes STRING in the TensorDataType enum but explicitly states 'We do not foresee strings as parameters' (adapter_schema.fbs). Without this check, a LoRA file with data_type=STRING (value 8) would pass all existing guards: - Size check: sizeof(std::string)*N bytes satisfies shape.Size()*elem_type->Size() - Non-owning Tensor init: buffer_deleter_ is null, so placement-new for std::string is skipped, leaving attacker-controlled raw bytes as fake std::string objects - Any subsequent string copy (CPUDataTransfer::CopyTensor) dereferences the fake string's internal pointer field, writing to an attacker- supplied address Fix: extend the existing data_type validation to also reject STRING. Added regression test CreateOrtValueOverLoraParameter_StringDataType that constructs a STRING-typed parameter with correctly-sized raw_data and verifies CreateOrtValueOverLoraParameter throws before any Tensor is built. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens LoRA adapter parameter loading by rejecting TensorDataType::STRING (in addition to UNDEFINED) when creating an OrtValue over adapter parameter data, preventing unsafe construction of non-owning tensors over raw bytes that could later be treated as std::string objects.
Changes:
- Extend
CreateOrtValueOverLoraParametervalidation to rejectSTRINGand emit a clearer “unsupported type” message. - Add a regression test that constructs a STRING-typed parameter with correctly sized
raw_dataand verifies the function throws before building a Tensor.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| onnxruntime/lora/adapter_format_utils.cc | Adds explicit rejection of TensorDataType::STRING with security rationale in comments. |
| onnxruntime/test/lora/lora_test.cc | Adds regression test covering STRING data_type rejection path. |
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
The fix is correct and the threat model in the description checks out. CreateOrtValueOverLoraParameter builds a non-owning Tensor directly over the flatbuffer's raw_data, so no placement-new runs; with data_type == STRING, elem_type->Size() is sizeof(std::string), which makes the expected_raw_data_size check pass for a crafted file and exposes raw file bytes as std::string objects. Rejecting STRING at the type guard closes this before any Tensor is constructed.
The regression test is also well designed: sizing raw_data to exactly 2 * sizeof(std::string) means the existing size check would not fire, which proves the new type guard is what rejects the input. Verified clang-format is clean on both files.
Two hardening suggestions inline (denylist vs. allowlist, and test assertion strength), plus one out-of-diff nitpick below. Nothing blocking.
Nitpick (outside the diff): the writer path still accepts STRING. AdapterFormatBuilder::AddParameter / SaveLoraParameter in onnxruntime/lora/adapter_format_utils.{h,cc} take TensorDataType unvalidated, so ORT can serialize an adapter that it will then refuse to load. Reusing the same predicate on the write path would keep both directions consistent and surface the error at authoring time rather than at load time.
I did not repeat the two already-resolved automated-review threads (the hard-coded = 64 bytes comment and the "write to attacker-supplied addresses" wording).
Reject non-trivial, packed, boolean, complex, and undeclared data types before constructing non-owning tensors over adapter bytes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
The follow-up commit fully addresses my prior feedback. The explicit allowlist now fails closed for undeclared enum values and excludes STRING, BOOL, complex, and packed representations before type lookup, byte-count validation, or non-owning tensor construction. The accepted set remains limited to fixed-size, unpacked numeric scalar types.
The shared rejection helper also verifies the intended guard message, and the new cases cover the distinct unsafe classes: non-trivial STRING, invalid-byte BOOL, UNDEFINED, and an undeclared value that would otherwise map to a packed runtime type. I found no remaining code issues.
I also checked the three red CI jobs. The Linux and Windows CUDA plugin jobs both fail the unrelated GatherBlockQuantized numerical test; the ARM64 job compiled both changed LoRA translation units successfully and then stopped during late linking without a compiler diagnostic. None points to this change.
This pull request hardens LoRA adapter parameter loading by accepting only fixed-size, unpacked scalar data types that are safe to expose through a non-owning tensor over raw FlatBuffer bytes.
Data-type validation:
UNDEFINED/STRINGdenylist with an explicit allowlist.STRING,BOOL, complex types, packed sub-byte types,UNDEFINED, and undeclared enum values.FlatBuffers validates the enum field's representation but not that its value is a declared
TensorDataTypemember. The allowlist therefore preserves the loader's invariant that storage size equals logical element count multiplied by element size, and remains safe if new schema values are added later.Tests:
STRINGwith correctly platform-sized raw data.UNDEFINED,BOOL, and an undeclared enum value.