Skip to content

Commit d67aa29

Browse files
authored
Revert "Arm backend: bounds-check the Ethos-U vela_bin_stream parser" (pytorch#23319)
Reverts pytorch#22580 There are internal buck failures: * buck test [@fbcode](https://www.internalfb.com/intern/profile/fbcode)//mode/dev fbcode//executorch/backends/arm/test:vela_external_blocks_test -- --exact 'fbcode//executorch/backends/arm/test:vela_external_blocks_test - VelaExternalBlocksTest.RejectsIoCountExceedingPayload' * buck test [@fbcode](https://www.internalfb.com/intern/profile/fbcode)//mode/dev fbcode//executorch/backends/arm/test:vela_external_blocks_test -- --exact 'fbcode//executorch/backends/arm/test:vela_external_blocks_test - VelaExternalBlocksTest.RejectsTruncatedBlockHeader' * buck test [@fbcode](https://www.internalfb.com/intern/profile/fbcode)//mode/dev fbcode//executorch/backends/arm/test:vela_external_blocks_test -- --exact 'fbcode//executorch/backends/arm/test:vela_external_blocks_test - VelaExternalBlocksTest.RejectsBlockSizeLargerThanBuffer' ``` est was added in target revision (5e0e2a95eb84059750928c5c889209358bd78854) hence is not present in base rev (f4903d106263140c16dd7db3a27803b9f59142f2) Note: Google Test filter = VelaExternalBlocksTest.RejectsIoCountExceedingPayload [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from VelaExternalBlocksTest [ RUN ] VelaExternalBlocksTest.RejectsIoCountExceedingPayload ExecuTorch PAL must be initialized before call to et_pal_current_ticks()*** ``` cc @digantdesai @freddan80 @per @zingo @oscarandersson8218 @mansnils @Sebastian-Larsson @robell @rascani
1 parent f15a41a commit d67aa29

4 files changed

Lines changed: 12 additions & 206 deletions

File tree

‎backends/arm/runtime/EthosUBackend.cpp‎

Lines changed: 2 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -189,21 +189,6 @@ class EthosUBackend final : public ::executorch::runtime::BackendInterface {
189189
const int input_count = handles.inputs ? handles.inputs->count : 0;
190190
const int output_count = handles.outputs ? handles.outputs->count : 0;
191191

192-
// input_count/output_count originate from the (potentially malformed)
193-
// delegate blob and index into args below (args[i] and args[input_count +
194-
// i]); reject anything that would read past the args span.
195-
if (input_count < 0 || output_count < 0 ||
196-
static_cast<size_t>(input_count) + static_cast<size_t>(output_count) >
197-
args.size()) {
198-
ET_LOG(
199-
Error,
200-
"Ethos-U IO count (%d in + %d out) exceeds args span size %zu",
201-
input_count,
202-
output_count,
203-
args.size());
204-
return Error::InvalidProgram;
205-
}
206-
207192
char* ethosu_scratch = nullptr;
208193
if (needs_scratch_allocation()) {
209194
MemoryAllocator* temp_allocator = context.get_temp_allocator();
@@ -274,22 +259,8 @@ class EthosUBackend final : public ::executorch::runtime::BackendInterface {
274259
}
275260

276261
if (needs_scratch_allocation()) {
277-
// The offset comes from the (potentially malformed) delegate blob and
278-
// is used as a memcpy destination; reject anything that would write
279-
// outside the scratch buffer.
280-
const int io_offset = handles.inputs->io[i].offset;
281-
if (io_offset < 0 ||
282-
static_cast<size_t>(io_offset) + tensor_in.nbytes() >
283-
handles.scratch_data_size) {
284-
ET_LOG(
285-
Error,
286-
"Input %d scratch offset %d + size %zu out of range for scratch size %zu",
287-
i,
288-
io_offset,
289-
tensor_in.nbytes(),
290-
handles.scratch_data_size);
291-
return Error::InvalidProgram;
292-
}
262+
char* scratch_addr = ethosu_scratch + handles.inputs->io[i].offset;
263+
293264
// Select a compatible copy routine including checking for input layouts
294265
// which require permutation.
295266
bool both_int = tensor_in.scalar_type() == ScalarType::Int &&
@@ -303,7 +274,6 @@ class EthosUBackend final : public ::executorch::runtime::BackendInterface {
303274
(handles.inputs->io[i].elem_size == 1);
304275

305276
if (both_char || both_int || both_short || both_bool) {
306-
char* scratch_addr = ethosu_scratch + io_offset;
307277
EXECUTORCH_PROF_SCOPE(
308278
event_tracer, "+EthosUBackend::execute()handles.input.memcpy()");
309279
// Sizes match and elt size matches so memcpy.

‎backends/arm/runtime/EthosUBackend_Cortex_M.cpp‎

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -129,10 +129,7 @@ Error platform_execute(
129129
// Write outputs from scratch into EValue pointers
130130
for (int i = 0; i < output_count; i++) {
131131
int tensor_count = 1, io_count = 1;
132-
// The offset comes from the (potentially malformed) delegate blob and is
133-
// used as the source of a copy out of the scratch buffer; reject anything
134-
// that would read outside it.
135-
const int out_offset = handles.outputs->io[i].offset;
132+
const char* output_addr = ethosu_scratch + handles.outputs->io[i].offset;
136133
// Process input EValue into scratch
137134
// Outputs are in the index immediately after inputs
138135
auto tensor_out = args[input_count + i]->toTensor();
@@ -141,19 +138,6 @@ Error platform_execute(
141138
tensor_out, &handles.outputs->io[i], &tensor_count, &io_count);
142139

143140
size_t tensor_bytes = tensor_out.nbytes();
144-
if (out_offset < 0 ||
145-
static_cast<size_t>(out_offset) + tensor_bytes >
146-
handles.scratch_data_size) {
147-
ET_LOG(
148-
Error,
149-
"Output %d scratch offset %d + size %zu out of range for scratch size %zu",
150-
i,
151-
out_offset,
152-
tensor_bytes,
153-
handles.scratch_data_size);
154-
return Error::InvalidProgram;
155-
}
156-
const char* output_addr = ethosu_scratch + out_offset;
157141
size_t io_bytes = static_cast<size_t>(io_count) *
158142
static_cast<size_t>(handles.outputs->io[i].elem_size);
159143

‎backends/arm/runtime/VelaBinStream.cpp‎

Lines changed: 7 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -62,44 +62,9 @@ Error resolve_external_block(
6262
return Error::Ok;
6363
}
6464

65-
// Reinterpret a block payload as a VelaIOs descriptor, but only after checking
66-
// that the payload is large enough to hold the `count` field and the full
67-
// `io[count]` array. Without this a malformed stream could point `count` past
68-
// the buffer, and every subsequent `io[i]` access (including the scratch
69-
// offsets used for memcpy at execute time) would read out of bounds.
70-
Error bind_io_block(const VelaBlockPayload& payload, VelaIOs** out) {
71-
if (payload.size < sizeof(VelaIOs)) {
72-
ET_LOG(Error, "Vela IO block too small for count field");
73-
return Error::InvalidProgram;
74-
}
75-
VelaIOs* ios = reinterpret_cast<VelaIOs*>(const_cast<char*>(payload.data));
76-
if (ios->count < 0) {
77-
ET_LOG(Error, "Negative Vela IO count %d", ios->count);
78-
return Error::InvalidProgram;
79-
}
80-
const size_t io_bytes = payload.size - sizeof(VelaIOs);
81-
if (static_cast<size_t>(ios->count) > io_bytes / sizeof(VelaIO)) {
82-
ET_LOG(
83-
Error,
84-
"Vela IO count %d overruns %zu payload bytes",
85-
ios->count,
86-
payload.size);
87-
return Error::InvalidProgram;
88-
}
89-
*out = ios;
90-
return Error::Ok;
91-
}
92-
9365
} // namespace
9466

9567
bool vela_bin_validate(const char* data, int size) {
96-
// A single block header must fit before computing the footer pointer;
97-
// otherwise foot = data + size - sizeof(VelaBinBlock) underflows below data
98-
// and the strncmp below reads out of bounds.
99-
if (size < static_cast<int>(sizeof(VelaBinBlock))) {
100-
ET_LOG(Error, "Vela binary too small: %d bytes", size);
101-
return false;
102-
}
10368
const char* foot = data + size - sizeof(VelaBinBlock);
10469

10570
// Check 16 byte alignment
@@ -131,46 +96,10 @@ Error vela_bin_read(
13196
const NamedDataMap* named_data_map,
13297
VelaHandles* handles) {
13398
const char* ptr = data;
134-
const size_t total = size < 0 ? 0 : static_cast<size_t>(size);
135-
136-
while (static_cast<size_t>(ptr - data) < total) {
137-
// A full block header must be present before any of its fields (size,
138-
// name, external) are read. The stream is attacker-controllable when the
139-
// delegate blob comes from a malformed .pte, so never dereference past the
140-
// buffer.
141-
const size_t remaining = total - static_cast<size_t>(ptr - data);
142-
if (remaining < sizeof(VelaBinBlock)) {
143-
ET_LOG(Error, "Truncated vela block header");
144-
return Error::InvalidProgram;
145-
}
99+
100+
while (ptr - data < size) {
146101
VelaBinBlock* b = reinterpret_cast<VelaBinBlock*>(const_cast<char*>(ptr));
147-
// The block's inline payload (padded to 16 bytes) must fit within the
148-
// bytes remaining after its header; otherwise b->data + b->size, and the
149-
// ptr advance below, would run off the end of the buffer.
150-
const size_t avail = remaining - sizeof(VelaBinBlock);
151-
// Check the raw size against the buffer first, in size_t, before padding.
152-
// next_mul_16() is computed in uintptr_t and wraps to 0 for b->size near
153-
// UINT32_MAX on 32-bit targets (i.e. Cortex-M), which would otherwise let a
154-
// ~4GB payload slip past a padded-only check.
155-
if (b->size > avail) {
156-
ET_LOG(
157-
Error,
158-
"Vela block size %u overruns %zu remaining payload bytes",
159-
static_cast<unsigned>(b->size),
160-
avail);
161-
return Error::InvalidProgram;
162-
}
163-
const size_t padded =
164-
(static_cast<size_t>(b->size) + 15) & ~static_cast<size_t>(15);
165-
if (padded > avail) {
166-
ET_LOG(
167-
Error,
168-
"Padded vela block size %zu overruns %zu remaining payload bytes",
169-
padded,
170-
avail);
171-
return Error::InvalidProgram;
172-
}
173-
ptr += sizeof(VelaBinBlock) + padded;
102+
ptr += sizeof(VelaBinBlock) + next_mul_16(b->size);
174103

175104
VelaBlockPayload payload{b->data, b->size};
176105
if (b->external == kVelaExternalBlockReference) {
@@ -189,9 +118,6 @@ Error vela_bin_read(
189118
return Error::InvalidProgram;
190119
} else if (!strncmp(b->name, "cmd_data", strlen("cmd_data"))) {
191120
// This driver magic header confirms a valid command stream in binary
192-
if (payload.size < strlen("COP1")) {
193-
return Error::InvalidProgram;
194-
}
195121
if (strncmp(payload.data, "COP1", strlen("COP1")) &&
196122
strncmp(payload.data, "COP2", strlen("COP2"))) {
197123
return Error::InvalidProgram;
@@ -202,23 +128,15 @@ Error vela_bin_read(
202128
handles->weight_data = payload.data;
203129
handles->weight_data_size = payload.size;
204130
} else if (!strncmp(b->name, "scratch_size", strlen("scratch_size"))) {
205-
if (payload.size < sizeof(uint32_t)) {
206-
ET_LOG(Error, "Malformed scratch_size block");
207-
return Error::InvalidProgram;
208-
}
209131
const uint32_t* scratch_size_ptr =
210132
reinterpret_cast<const uint32_t*>(payload.data);
211133
handles->scratch_data_size = *scratch_size_ptr;
212134
} else if (!strncmp(b->name, "inputs", strlen("inputs"))) {
213-
const Error status = bind_io_block(payload, &handles->inputs);
214-
if (status != Error::Ok) {
215-
return status;
216-
}
135+
handles->inputs =
136+
reinterpret_cast<VelaIOs*>(const_cast<char*>(payload.data));
217137
} else if (!strncmp(b->name, "outputs", strlen("outputs"))) {
218-
const Error status = bind_io_block(payload, &handles->outputs);
219-
if (status != Error::Ok) {
220-
return status;
221-
}
138+
handles->outputs =
139+
reinterpret_cast<VelaIOs*>(const_cast<char*>(payload.data));
222140
} else if (!strncmp(
223141
b->name, "vela_end_stream", strlen("vela_end_stream"))) {
224142
// expect vela_end_stream last

‎backends/arm/test/vela_external_blocks_test.cpp‎

Lines changed: 2 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
using executorch::backends::arm::kVelaExternalBlockReference;
2727
using executorch::backends::arm::vela_bin_read;
2828
using executorch::backends::arm::VelaHandles;
29-
using executorch::backends::arm::VelaIO;
3029
using executorch::backends::arm::VelaIOs;
3130
using executorch::runtime::Error;
3231
using executorch::runtime::FreeableBuffer;
@@ -262,14 +261,8 @@ TEST(VelaExternalBlocksTest, ResolvesEveryExternalBlockType) {
262261
}
263262

264263
TEST(VelaExternalBlocksTest, ResolvesMultipleExternalBlocksIndependently) {
265-
// The payload must be large enough to hold `count` VelaIO entries, otherwise
266-
// vela_bin_read rejects it. count is the first int of the buffer.
267-
alignas(16) std::array<uint8_t, sizeof(VelaIOs) + sizeof(VelaIO)>
268-
inputs_data{};
269-
inputs_data[0] = 1; // count = 1
270-
alignas(16) std::array<uint8_t, sizeof(VelaIOs) + 2 * sizeof(VelaIO)>
271-
outputs_data{};
272-
outputs_data[0] = 2; // count = 2
264+
alignas(16) std::array<uint8_t, 4> inputs_data{1, 0, 0, 0};
265+
alignas(16) std::array<uint8_t, 4> outputs_data{2, 0, 0, 0};
273266
std::vector<uint8_t> stream;
274267
append_block(stream, "vela_bin_stream");
275268
append_block(
@@ -318,62 +311,3 @@ TEST(VelaExternalBlocksTest, RejectsUnknownExternalBlock) {
318311
&handles),
319312
Error::InvalidProgram);
320313
}
321-
322-
// A block header that is truncated by the end of the buffer must be rejected
323-
// instead of being read out of bounds.
324-
TEST(VelaExternalBlocksTest, RejectsTruncatedBlockHeader) {
325-
std::vector<uint8_t> stream;
326-
append_block(stream, "vela_bin_stream");
327-
// Fewer than sizeof(VelaBinBlock) trailing bytes: not a full header.
328-
stream.insert(stream.end(), 8, 0);
329-
VelaHandles handles{};
330-
331-
EXPECT_EQ(
332-
vela_bin_read(
333-
reinterpret_cast<const char*>(stream.data()),
334-
stream.size(),
335-
nullptr,
336-
&handles),
337-
Error::InvalidProgram);
338-
}
339-
340-
// A block whose declared size field exceeds the bytes remaining in the buffer
341-
// must be rejected before its payload (or the cursor advance) runs off the end.
342-
TEST(VelaExternalBlocksTest, RejectsBlockSizeLargerThanBuffer) {
343-
std::vector<uint8_t> stream;
344-
append_block(stream, "vela_bin_stream");
345-
// Hand-write a header claiming a 4096-byte payload but supply only one byte.
346-
append_fixed_string(stream, "weight_data", 16);
347-
append_u32(stream, 4096u);
348-
stream.push_back(0); // external
349-
stream.resize(stream.size() + 11, 0); // reserved
350-
stream.push_back(0xAA); // single real payload byte
351-
VelaHandles handles{};
352-
353-
EXPECT_EQ(
354-
vela_bin_read(
355-
reinterpret_cast<const char*>(stream.data()),
356-
stream.size(),
357-
nullptr,
358-
&handles),
359-
Error::InvalidProgram);
360-
}
361-
362-
// An inputs/outputs block whose declared count is larger than the payload can
363-
// hold must be rejected, so that io[i] is never read out of bounds at execute.
364-
TEST(VelaExternalBlocksTest, RejectsIoCountExceedingPayload) {
365-
std::vector<uint8_t> stream;
366-
append_block(stream, "vela_bin_stream");
367-
// count = 1 but no VelaIO entry follows (only the 4-byte count field).
368-
append_block(stream, "inputs", std::vector<uint8_t>{1, 0, 0, 0});
369-
append_block(stream, "vela_end_stream");
370-
VelaHandles handles{};
371-
372-
EXPECT_EQ(
373-
vela_bin_read(
374-
reinterpret_cast<const char*>(stream.data()),
375-
stream.size(),
376-
nullptr,
377-
&handles),
378-
Error::InvalidProgram);
379-
}

0 commit comments

Comments
 (0)