From 8971b321929130c222b6696cdb41a57fc5107808 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Fri, 27 Dec 2019 16:32:59 -0800 Subject: [PATCH 1/3] Fix Updater potential overflow, add host tests Fixes #4674 The Updater class could, when exactly 4K bytes were in the buffer but not yet written to flash, allow overwriting data written to it beyond the passed-in size parameter. Fix per @jason-but's suggestion, and add a host test (plus minor changes to Updater code to support host testing). --- cores/esp8266/Updater.cpp | 16 ++++++++-- tests/host/Makefile | 5 ++- tests/host/core/test_Updater.cpp | 55 ++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 4 deletions(-) create mode 100644 tests/host/core/test_Updater.cpp diff --git a/cores/esp8266/Updater.cpp b/cores/esp8266/Updater.cpp index ebb893aabd..0a8a165dbc 100644 --- a/cores/esp8266/Updater.cpp +++ b/cores/esp8266/Updater.cpp @@ -104,7 +104,9 @@ bool UpdaterClass::begin(size_t size, int command, int ledPin, uint8_t ledOn) { _reset(); clearError(); // _error = 0 +#ifndef HOST_MOCK wifi_set_sleep_type(NONE_SLEEP_T); +#endif //address where we will start writing the update uintptr_t updateStartAddress = 0; @@ -115,7 +117,11 @@ bool UpdaterClass::begin(size_t size, int command, int ledPin, uint8_t ledOn) { if (command == U_FLASH) { //address of the end of the space available for sketch and update +#ifdef HOST_MOCK + uintptr_t updateEndAddress = (uintptr_t) -1; +#else uintptr_t updateEndAddress = (uintptr_t)&_FS_start - 0x40200000; +#endif updateStartAddress = (updateEndAddress > roundedSize)? (updateEndAddress - roundedSize) : 0; @@ -132,10 +138,12 @@ bool UpdaterClass::begin(size_t size, int command, int ledPin, uint8_t ledOn) { } } else if (command == U_FS) { +#ifndef HOST_MOCK if((uintptr_t)&_FS_start + roundedSize > (uintptr_t)&_FS_end) { _setError(UPDATE_ERROR_SPACE); return false; } +#endif #ifdef ATOMIC_FS_UPDATE //address of the end of the space available for update @@ -148,7 +156,11 @@ bool UpdaterClass::begin(size_t size, int command, int ledPin, uint8_t ledOn) { return false; } #else + #ifdef HOST_MOCK + updateStartAddress = 0; + #else updateStartAddress = (uintptr_t)&_FS_start - 0x40200000; + #endif #endif } else { @@ -378,9 +390,7 @@ size_t UpdaterClass::write(uint8_t *data, size_t len) { if(hasError() || !isRunning()) return 0; - if(len > remaining()){ - //len = remaining(); - //fail instead + if(progress() + _bufferLen + len > _size) { _setError(UPDATE_ERROR_SPACE); return 0; } diff --git a/tests/host/Makefile b/tests/host/Makefile index aaf8a88d91..119e8f7927 100644 --- a/tests/host/Makefile +++ b/tests/host/Makefile @@ -70,6 +70,7 @@ CORE_CPP_FILES := $(addprefix $(CORE_PATH)/,\ Schedule.cpp \ HardwareSerial.cpp \ crc32.cpp \ + Updater.cpp \ ) \ $(addprefix $(LIBRARIES_PATH)/ESP8266SdFat/src/, \ FatLib/FatFile.cpp \ @@ -98,6 +99,7 @@ MOCK_CPP_FILES_COMMON := $(addprefix common/,\ MockUART.cpp \ MockTools.cpp \ MocklwIP.cpp \ + MockDigital.cpp \ ) MOCK_CPP_FILES := $(MOCK_CPP_FILES_COMMON) $(addprefix common/,\ @@ -136,7 +138,8 @@ TEST_CPP_FILES := \ core/test_md5builder.cpp \ core/test_string.cpp \ core/test_PolledTimeout.cpp \ - core/test_Print.cpp + core/test_Print.cpp \ + core/test_Updater.cpp PREINCLUDES := \ -include common/mock.h \ diff --git a/tests/host/core/test_Updater.cpp b/tests/host/core/test_Updater.cpp new file mode 100644 index 0000000000..8985067b34 --- /dev/null +++ b/tests/host/core/test_Updater.cpp @@ -0,0 +1,55 @@ +/* + test_Updater.cpp - Updater tests + Copyright © 2019 Earle F. Philhower, III + + Permission is hereby granted, free of charge, to any person obtaining a copy + of this software and associated documentation files (the "Software"), to deal + in the Software without restriction, including without limitation the rights + to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + copies of the Software, and to permit persons to whom the Software is + furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice shall be included in + all copies or substantial portions of the Software. + */ + +#include +#include + + +// Use a SPIFFS file because we can't instantiate a virtual class like Print +TEST_CASE("Updater fails when writes overflow requested size", "[core][Updater]") +{ + UpdaterClass *u; + uint8_t buff[6000]; + u = new UpdaterClass(); + REQUIRE(u->begin(6000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->write(buff, 1000)); + REQUIRE(u->remaining() == 0); + // All bytes written, should fail next + REQUIRE(!u->write(buff, 1000)); + delete u; + + // Updater to a 4K aligned size, check nomissing over/underflow + u = new UpdaterClass(); + REQUIRE(u->begin(8192)); + REQUIRE(u->remaining() == 8192); + REQUIRE(u->write(buff, 4096)); + REQUIRE(u->write(buff, 4096)); + REQUIRE(!u->write(buff, 1)); + delete u; + + // Issue #4674 + u = new UpdaterClass(); + REQUIRE(u->begin(5000)); + REQUIRE(u->write(buff, 2048)); + REQUIRE(u->write(buff, 2048)); + // Should fail, would write 6KB + REQUIRE(!u->write(buff, 2048)); + delete u; +} From 70ab32e57be86d81083eeb671feb4cfc84224b6e Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Fri, 27 Dec 2019 16:37:47 -0800 Subject: [PATCH 2/3] Add missed mock file --- tests/host/common/MockDigital.cpp | 56 +++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 tests/host/common/MockDigital.cpp diff --git a/tests/host/common/MockDigital.cpp b/tests/host/common/MockDigital.cpp new file mode 100644 index 0000000000..aa04527ab5 --- /dev/null +++ b/tests/host/common/MockDigital.cpp @@ -0,0 +1,56 @@ +/* + digital.c - wiring digital implementation for esp8266 + + Copyright (c) 2015 Hristo Gochkov. All rights reserved. + This file is part of the esp8266 core for Arduino environment. + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with this library; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +*/ +#define ARDUINO_MAIN +#include "wiring_private.h" +#include "pins_arduino.h" +#include "c_types.h" +#include "eagle_soc.h" +#include "ets_sys.h" +#include "user_interface.h" +#include "core_esp8266_waveform.h" +#include "interrupts.h" + +extern "C" { + +static uint8_t _mode[17]; +static uint8_t _gpio[17]; + +extern void pinMode(uint8_t pin, uint8_t mode) { + if (pin < 17) { + _mode[pin] = mode; + } +} + +extern void digitalWrite(uint8_t pin, uint8_t val) { + if (pin < 17) { + _gpio[pin] = val; + } +} + +extern int digitalRead(uint8_t pin) { + if (pin < 17) { + return _gpio[pin] != 0; + } else { + return 0; + } +} + +}; From 13aef12fb926543fef5326d9ccd187089c523f91 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Fri, 27 Dec 2019 17:45:55 -0800 Subject: [PATCH 3/3] Remove most testing ifdefs fro updater Per @mcspr's suggestion, we can pass in fake link symbols allowing Updater to take the address of `_FS_start`/etc. even when building on the host for testing. There is still a single remaining wifi_set_power_mode ifdef'd and a duplication of the digitalWrite/pinMode for testing vs. host building. --- cores/esp8266/Updater.cpp | 10 ---------- tests/host/Makefile | 3 ++- tests/host/core/test_Updater.cpp | 1 + 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/cores/esp8266/Updater.cpp b/cores/esp8266/Updater.cpp index 0a8a165dbc..70d16a55ed 100644 --- a/cores/esp8266/Updater.cpp +++ b/cores/esp8266/Updater.cpp @@ -117,11 +117,7 @@ bool UpdaterClass::begin(size_t size, int command, int ledPin, uint8_t ledOn) { if (command == U_FLASH) { //address of the end of the space available for sketch and update -#ifdef HOST_MOCK - uintptr_t updateEndAddress = (uintptr_t) -1; -#else uintptr_t updateEndAddress = (uintptr_t)&_FS_start - 0x40200000; -#endif updateStartAddress = (updateEndAddress > roundedSize)? (updateEndAddress - roundedSize) : 0; @@ -138,12 +134,10 @@ bool UpdaterClass::begin(size_t size, int command, int ledPin, uint8_t ledOn) { } } else if (command == U_FS) { -#ifndef HOST_MOCK if((uintptr_t)&_FS_start + roundedSize > (uintptr_t)&_FS_end) { _setError(UPDATE_ERROR_SPACE); return false; } -#endif #ifdef ATOMIC_FS_UPDATE //address of the end of the space available for update @@ -156,11 +150,7 @@ bool UpdaterClass::begin(size_t size, int command, int ledPin, uint8_t ledOn) { return false; } #else - #ifdef HOST_MOCK - updateStartAddress = 0; - #else updateStartAddress = (uintptr_t)&_FS_start - 0x40200000; - #endif #endif } else { diff --git a/tests/host/Makefile b/tests/host/Makefile index 119e8f7927..f6361f36aa 100644 --- a/tests/host/Makefile +++ b/tests/host/Makefile @@ -6,6 +6,7 @@ LIBRARIES_PATH := ../../libraries FORCE32 ?= 1 OPTZ ?= -Os V ?= 0 +DEFSYM_FS ?= -Wl,--defsym,_FS_start=0x40300000 -Wl,--defsym,_FS_end=0x411FA000 -Wl,--defsym,_FS_page=0x100 -Wl,--defsym,_FS_block=0x2000 -Wl,--defsym,_EEPROM_start=0x411fb000 MAKEFILE = $(word 1, $(MAKEFILE_LIST)) @@ -236,7 +237,7 @@ $(BINDIR)/core.a: $(C_OBJECTS) $(CPP_OBJECTS_CORE) ranlib -c $@ $(OUTPUT_BINARY): $(CPP_OBJECTS_TESTS) $(BINDIR)/core.a - $(VERBLD) $(CXX) $(LDFLAGS) $^ -o $@ + $(VERBLD) $(CXX) $(DEFSYM_FS) $(LDFLAGS) $^ -o $@ ################################################# # building ino sources diff --git a/tests/host/core/test_Updater.cpp b/tests/host/core/test_Updater.cpp index 8985067b34..f3b850e130 100644 --- a/tests/host/core/test_Updater.cpp +++ b/tests/host/core/test_Updater.cpp @@ -22,6 +22,7 @@ TEST_CASE("Updater fails when writes overflow requested size", "[core][Updater]" { UpdaterClass *u; uint8_t buff[6000]; + memset(buff, 0, sizeof(buff)); u = new UpdaterClass(); REQUIRE(u->begin(6000)); REQUIRE(u->write(buff, 1000));