Skip to content

tmfile: bounds-check offsets in the tm2 tensor loader (#1449) - #1450

Open
professor-moody wants to merge 2 commits into
OAID:tengine-litefrom
professor-moody:fix-tmfile-oob-reads
Open

tmfile: bounds-check offsets in the tm2 tensor loader (#1449)#1450
professor-moody wants to merge 2 commits into
OAID:tengine-litefrom
professor-moody:fix-tmfile-oob-reads

Conversation

@professor-moody

Copy link
Copy Markdown

Follows up #1449.

As reported there, the tmfile (tm2) loader dereferences offsets and counts straight from the file without checking them against the mapped size, so a crafted model triggers out-of-bounds reads while loading.

This adds a small helper, tm2_off_ok(priv, off, sz), that returns true only when [off, off+sz) is fully inside the mapped file (priv->mem_len is already stored by both the file and memory load paths), and routes the reads in load_graph_tensors through it: the tensor and buffer offset tables, the per-tensor offset, the tensor name string (offset and length), the dims vector, and the buffer_id index (checked against v_num before indexing the offsets array).

I started with load_graph_tensors since that is where most of the crashes landed. The same helper should be applied to the base-pointer resolution (get_tm_file_header/model/subgraph) and the node and IO loaders (load_graph_nodes, set_graph_io_nodes, load_graph_sub_info) to close the remaining sites. Happy to extend this PR to cover those, or split it, whatever is easiest to review.

Verified on tengine-lite HEAD that a build with this patch rejects the crafted models that previously crashed in the tensor loader, and still loads the shipped benchmark models normally.

The tm2 loader dereferenced file offsets and counts without checking them
against the mapped size, so a crafted model caused out-of-bounds reads on
load (OAID#1449). Add tm2_off_ok() and route the load_graph_tensors reads through
it: the tensor/buffer offset tables, the per-tensor offset, the name string
(offset and length), the dims vector, and the buffer_id index.
The previous commit did not route offect_vo_quantparams through tm2_off_ok,
so the quant-param block at the tail of load_graph_tensors still dereferenced
a file-controlled offset off mem_base. A crafted tmfile therefore still gets a
heap out-of-bounds read inside the function this branch set out to bound:

  AddressSanitizer: heap-buffer-overflow, READ of size 4
    #0 load_graph_tensors  tm2_serializer.c:470
    OAID#1 load_graph          tm2_serializer.c:875
    OAID#2 load_model          tm2_serializer.c:941
    OAID#3 create_graph        c_api.c:429

Reproduced on x86-64 with clang and ASan against the shipped
googlenet_benchmark.tmfile with two bytes changed, so that
tensor[0].offect_vo_quantparams points past the end of the file.

Three dereferences in that block were unguarded, not one: the
TM2_Vector_offsets header, offsets[0] on the single-param path, and
offsets[j] in the multi-param loop. The offsets[] array extent that v_num
describes was unbounded too; that bound is written as a division rather than
a multiply so it cannot overflow, matching the subtraction form in
tm2_off_ok.

I missed this the first time because the check I ran, that the shipped
benchmark models still load, cannot exercise this code at all: all 14 of
them have offect_vo_quantparams == TM2_NOT_SET on every tensor, so none of
them enters the block.

Verified after the change: the crafted file is rejected instead of faulting,
all 14 shipped models still load, and a model carrying legitimate in-bounds
quant params (v_num=1, real scale and zero_point) still loads and runs, so
the new bounds reject only out-of-range offsets.
@professor-moody

Copy link
Copy Markdown
Author

Pushed a second commit. Please review the branch rather than the first commit: testing my own patch against a wider set of crafted files showed it did not close the bug in the function it was meant to bound.

offect_vo_quantparams never went through tm2_off_ok, so the quant-param block at the tail of load_graph_tensors still dereferences a file-controlled offset off mem_base. With the first commit applied, a crafted tmfile still faults:

AddressSanitizer: heap-buffer-overflow, READ of size 4
  #0 load_graph_tensors  tm2_serializer.c:470
  #1 load_graph          tm2_serializer.c:875
  #2 load_model          tm2_serializer.c:941
  #3 create_graph        c_api.c:429

Reproduced on x86-64 with clang and ASan. The crafted file is your shipped benchmark/models/googlenet_benchmark.tmfile with two bytes changed so that tensor[0].offect_vo_quantparams points past the end of the file. Pristine faults at :441, first commit faults at :470, so the fault moved with the inserted lines rather than being fixed.

Three dereferences in that block were unguarded, not one: the TM2_Vector_offsets header, offsets[0] on the single-param path, and offsets[j] in the multi-param loop. The offsets[] array extent that v_num describes was unbounded as well. That bound is written as a division rather than a multiply so it cannot overflow, matching the subtraction form already in tm2_off_ok.

Worth flagging why I missed it, since it affects how you might want to test the branch: the check I ran was that the shipped benchmark models still load normally. That check cannot exercise this code at all. All 14 shipped models have offect_vo_quantparams == TM2_NOT_SET on every tensor, so none of them enters the block. It passed without ever running the lines in question.

After the fixup:

  • the crafted file is rejected rather than faulting
  • all 14 shipped benchmark models still load, none regressed
  • a model carrying legitimate in-bounds quant params (v_num = 1, real scale and zero_point) still loads and runs

That last one matters more than the others: the new bounds reject only offsets that fall outside the mapping, not quantized models generally.

One unrelated thing I ran into that you may not be aware of, and which is not part of this PR. Every run of tm_benchmark under ASan reports a stack buffer overflow in the online reporting code, independent of the model:

AddressSanitizer: stack-buffer-overflow
  #0 vsprintf
  #1 sprintf
  #2 md5_to_string       source/olreport/md5.c:169
  #3 general_report_data  source/olreport/reportdata.c:184
  #4 do_real_online_report source/olreport/onlinereportmgr.c:100

It fires on a clean shipped model with no crafted input at all. Happy to open a separate issue if that is useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant