diff --git a/sdk/PATCHES.md b/sdk/PATCHES.md index 1077e86..7329f9f 100644 --- a/sdk/PATCHES.md +++ b/sdk/PATCHES.md @@ -9,6 +9,28 @@ Lib/GNU, Bin/secboot.img, Tools/GNU/link_w600_2m.ld (→ ld/) und wm_tool.c (→ (Header + Payload + 512-Byte-Endblock) bleibt unverändert. Die starke Definition des Hooks liegt seit Plan 3 in `src/sys/nl_ota.c` (Trailer-Prüfung über `nl_trailer_verify()`); die bisherige `weak`-Variante `src/sys/fwup_hook_default.c` ist entfallen. + Fix-Runde 1 (Task-4-Review) hat drei weitere Stellen in diesem Bereich korrigiert: + - `wm_fwup.c`, Bedingung um `nl_fwup_verify_tail()`: Checksummen- und Trailer-Prüfung liefen in + einer kurzschließenden `||`-Bedingung; bei falscher Checksumme wurde `nl_fwup_verify_tail()` nie + aufgerufen. Beide Fälle jetzt getrennt behandelt; ein Checksummenfehler ruft + `nl_ota_set_error("crc")` auf, bevor `goto request_finish` springt (sonst hätte `ota_task()` ihn + als generisches `"download"` gemeldet). + - `wm_fwup.c`, `request_finish:`-Block: `tls_sys_reset()` lief unbedingt, sobald der Transfer zu + Ende war - auch wenn `nl_fwup_verify_tail()` oder die Checksummenprüfung das Image gerade + abgelehnt hatten. Die Lampe hätte also trotz abgelehntem Image sofort neu gestartet, `ota_task()` + wäre nie zu Ende gelaufen und `last_ota_error` hätte nie `trailer`/`device_mismatch`/`hash` + gemeldet. Der Reset läuft jetzt nur noch, wenn `fwup->current_state` keinen Fehler-Bit-Satz + (`TLS_FWUP_STATE_ERROR`) trägt. + - `Src/App/OTA/wm_http_fwup.c`, `HANDLE_BODY`: Scheitert `tls_fwup_img_header_check()`, blieb + `nRetCode` auf `HTTP_CLIENT_SUCCESS` stehen, wodurch die abschließende + `if(nRetCode) socket_fwup_err(...)`-Bereinigung nie lief und die SDK-eigene fwup-Session + (`fwup->busy`) für jeden weiteren Versuch blockiert blieb. `nRetCode` wird jetzt in diesem Fall + explizit auf `HTTP_CLIENT_ERROR_BAD_HEADER` gesetzt. + - `Src/App/OTA/wm_http_fwup.c`, `SHUTDOWN_LINK`: Ein abbrechender Download sprang unbegrenzt oft + zurück nach `PREPARE_PACKET` (Resume per Range-Header) - schaltet der Server während der + Übertragung ab, hängt `t_http_fwup()` (und damit der aufrufende Task) für immer. Eine großzügige + Obergrenze (`NL_OTA_MAX_RETRY_ROUNDS`, 200 Resume-Runden) lässt `http_fwup()` danach mit + `HTTP_CLIENT_ERROR_SOCKET_TIME_OUT` zurückkehren, statt endlos zu warten. 2. `Src/OS/RTOS/include/FreeRTOSConfig.h`: `configGENERATE_RUN_TIME_STATS 0`, QConnex-Debug-Hooks entfernt. 3. `undefined reference` beim Linken auf `EarlyInit` und `pre_gpio_config` (von `sdk/Platform/Sys/wm_main.c` als `extern`-Hooks erwartet, aber nur von der App bereitgestellt): @@ -21,10 +43,19 @@ Lib/GNU, Bin/secboot.img, Tools/GNU/link_w600_2m.ld (→ ld/) und wm_tool.c (→ Ein Task-Deadlock (z.B. ein Task, der `nl_app_lock()` hält und nie freigibt) hätte den Watchdog damit nie ausgelöst, weil der Idle-Task munter weiter gefüttert hätte. Der `tls_watchdog_clr()`- Aufruf wurde entfernt; `wfi` bleibt. Der einzige verbleibende `tls_watchdog_clr()`-Aufruf im SDK - ist in `Platform/Common/fwup/wm_fwup.c` (Firmware-Update-Task) - das ist unkritisch, da er nur - während eines laufenden OTA-Updates feuert und den `app_task`-Heartbeat in `src/app_main.c` - nicht ersetzt (der Watchdog wird sonst ausschließlich dort über den `s_light_beat`-Heartbeat - des `light_task` gefüttert). Getestet mit dem Konsolenkommando `wdtest` + ist in `Platform/Common/fwup/wm_fwup.c` (Firmware-Update-Task). + **Einschätzung aktualisiert (Task 4, Fix-Runde 1):** Als dieser Punkt geschrieben wurde, gab es noch + keinen OTA-Pfad - das Fenster war rein theoretisch. Mit Task 4 ist es real und kann durch einen + langsamen oder hängenden Download mehrere Minuten dauern: Solange `wm_fwup.c` seinen eigenen + `tls_watchdog_clr()` aufruft, wird der Watchdog gefüttert, **egal** ob `light_task`, `nl_wifi`/`nl_mdns` + oder `nl_http_server` in dieser Zeit echt hängen - der App-Task-Heartbeat-Mechanismus in + `src/app_main.c` (der genau das eigentlich verhindern soll) greift währenddessen nicht, weil der + Watchdog gar nicht bei ihm ankommt. Das ist kein Rückschritt gegenüber vorher (vor Task 4 gab es + dieses Fenster schlicht nicht), aber die bisherige Einstufung „unkritisch" war nur so lange richtig, + wie der Fall real nie eintrat. Kein Fix in dieser Runde - der Watchdog-Heartbeat müsste dafür wissen, + ob der fwup-Task selbst noch vorankommt (z. B. über einen eigenen Beat-Zähler, den `app_task` + zusätzlich zu `light`/`net`/`http` prüft); das ist ein eigenständiger Task für einen späteren Plan. + Getestet mit dem Konsolenkommando `wdtest` (src/sys/nl_console_cmd.c, src/sys/nl_console.c): es hält `nl_app_lock()` für immer, wodurch `light_task` und `app_task` blockieren und der Watchdog den Chip zurücksetzen muss. 5. `Platform/Drivers/internalflash/wm_internal_fls.c`: Semaphor `s_fls2_lock` serialisiert den Flash-Pfad diff --git a/sdk/Platform/Common/fwup/wm_fwup.c b/sdk/Platform/Common/fwup/wm_fwup.c index 3894ccc..3c9b894 100644 --- a/sdk/Platform/Common/fwup/wm_fwup.c +++ b/sdk/Platform/Common/fwup/wm_fwup.c @@ -172,6 +172,7 @@ static void tls_fwup_img_update_header(T_BOOTER* img_param) /* ninlight: SHA256 + tail verification hook instead of the QConnex RSA check (see sdk/PATCHES.md) */ #include "util/nl_sha256.h" #include "sys/nl_fwup_hook.h" +#include "sys/nl_ota.h" /* nl_ota_set_error() - see sdk/PATCHES.md point 1 */ static void fwup_scheduler(void *data) @@ -440,9 +441,20 @@ static void fwup_scheduler(void *data) printf("%02x ", bptr[i]); printf("\n"); #endif - if (org_checksum != image_checksum || !nl_fwup_verify_tail(image_sha256, RSA_SHA256, 512)) // QC_CHANGE_MK + if (org_checksum != image_checksum) // QC_CHANGE_MK { + /* ninlight: a checksum failure never reaches nl_fwup_verify_tail() below, + * so report it here explicitly - otherwise last_ota_error would fall back + * to the generic "download" instead of the more precise "crc" (see + * sdk/PATCHES.md point 1). */ TLS_DBGPRT_ERR("varify incorrect[0x%02x, but 0x%02x]\n", org_checksum, image_checksum); + nl_ota_set_error("crc"); + request->status = TLS_FWUP_REQ_STATUS_FCRC; + fwup->current_state |= TLS_FWUP_STATE_ERROR_CRC; + goto request_finish; + } + else if (!nl_fwup_verify_tail(image_sha256, RSA_SHA256, 512)) // QC_CHANGE_MK + { request->status = TLS_FWUP_REQ_STATUS_FCRC; fwup->current_state |= TLS_FWUP_STATE_ERROR_CRC; goto request_finish; @@ -478,7 +490,14 @@ request_finish: { request->complete(request, request->arg); } - if(fwup->updated_len >= (fwup->total_len)) + /* ninlight: only reset when the image was actually accepted. Before this fix the + * condition below fired unconditionally once the transfer reached its end - + * including a request that got rejected by nl_fwup_verify_tail() (trailer/device/ + * hash mismatch) or by the checksum check above (goto request_finish also lands + * here). Resetting in that case would reboot into the OLD firmware and never let + * ota_task() report the failure - so last_ota_error stayed NULL forever. See + * sdk/PATCHES.md point 1. */ + if(fwup->updated_len >= (fwup->total_len) && !(fwup->current_state & TLS_FWUP_STATE_ERROR)) { fwup_update_autoflag(); tls_sys_reset(); diff --git a/sdk/Src/App/OTA/wm_http_fwup.c b/sdk/Src/App/OTA/wm_http_fwup.c index 97be5fc..53bbd82 100644 --- a/sdk/Src/App/OTA/wm_http_fwup.c +++ b/sdk/Src/App/OTA/wm_http_fwup.c @@ -16,9 +16,15 @@ enum ota_state {PREPARE_PACKET, SETUP_LINK_AND_REQ, RECV_RSP, HANDLE_HEADER, HANDLE_BODY, SHUTDOWN_LINK, QUIT_OTA}; +/* ninlight: SHUTDOWN_LINK resumes an unfinished transfer via PREPARE_PACKET forever - a server + * that closes the connection early (or never sends the full body) used to hang this task, and + * with it the caller of t_http_fwup(), permanently (see sdk/PATCHES.md point 1). Cap the number + * of resume rounds generously so a slow-but-healthy download still completes. */ +#define NL_OTA_MAX_RETRY_ROUNDS 200 + int http_fwup(HTTPParameters ClientParams) { - char* Buffer = NULL; + char* Buffer = NULL; char token[32]; char headRange[32] = {0}; int nRetCode = 0; @@ -26,6 +32,7 @@ int http_fwup(HTTPParameters ClientParams) u32 partLen; u32 totalLen = 0; u32 recvLen = 0; + u32 retryRounds = 0; T_BOOTER *booter; #if USE_BREAK_POINT_RESUME == 0 u32 breakFlag = 0; @@ -124,6 +131,13 @@ int http_fwup(HTTPParameters ClientParams) } else { + /* ninlight: nRetCode is still HTTP_CLIENT_SUCCESS here (the read itself + * worked, only the image header is bad), so the "if(nRetCode)" cleanup + * at the end of this function used to be skipped entirely - leaving + * socket_fwup_err()/tls_fwup_exit() uncalled and the fwup session stuck + * busy for every later attempt. Force the cleanup path (see + * sdk/PATCHES.md point 1). */ + nRetCode = HTTP_CLIENT_ERROR_BAD_HEADER; now_state = QUIT_OTA; break; } @@ -191,7 +205,16 @@ int http_fwup(HTTPParameters ClientParams) { if(pHTTP){ HTTPClientCloseRequest(&pHTTP); - (recvLen == totalLen)?(now_state = QUIT_OTA):(now_state = PREPARE_PACKET); + if (recvLen == totalLen) { + now_state = QUIT_OTA; + } else if (++retryRounds > NL_OTA_MAX_RETRY_ROUNDS) { + /* ninlight: give up instead of resuming forever, see the comment above + * NL_OTA_MAX_RETRY_ROUNDS. */ + nRetCode = HTTP_CLIENT_ERROR_SOCKET_TIME_OUT; + now_state = QUIT_OTA; + } else { + now_state = PREPARE_PACKET; + } } } break; diff --git a/src/app_main.c b/src/app_main.c index d2dc86b..b497394 100644 --- a/src/app_main.c +++ b/src/app_main.c @@ -165,7 +165,9 @@ static void app_task(void *arg) have_last = nl_state_log_load(&last); store_unlock(); if (s_settings.ota_pending) { - /* We came back from an update attempt: restore the light the user had on. */ + /* We came back from an update attempt (accepted or rejected). Just clear the flag so it + * doesn't stick in flash forever; nl_boot_state() below restores the light from `last`/ + * power_on_behaviour as on any other boot, no special-casing needed here. */ s_settings.ota_pending = 0; printf("boot after ota attempt\r\n"); store_lock(); diff --git a/src/net/nl_api.c b/src/net/nl_api.c index 6116406..c53d325 100644 --- a/src/net/nl_api.c +++ b/src/net/nl_api.c @@ -336,9 +336,13 @@ static void handle_command(const nl_api_ctx_t *ctx, const nl_http_request_t *req } else if (strcmp(type, "fw_update") == 0) { if (get_string(cmd, "url", 8, 255, &url) != 1 || strncmp(url, "http://", 7) != 0) reply_error(resp, 400, "invalid url"); - else if (ctx->fw_update(url) != 0) - reply_error(resp, 501, "fw_update not available"); - else + else if (ctx->fw_update(url) != 0) { + /* fw_update() itself can genuinely fail now (busy, or a rejected URL) - 409 with the + * reason, not a blanket "not implemented". */ + nl_sys_info_t in; + ctx->get_info(&in); + reply_error(resp, 409, in.last_ota_error ? in.last_ota_error : "fw_update busy or invalid url"); + } else reply(resp, 202, "{\"status\":\"updating\"}"); } else if (strcmp(type, "reboot") == 0) { reply(resp, 200, "{\"ok\":true}"); diff --git a/src/sys/nl_ota.c b/src/sys/nl_ota.c index 906b550..60731fa 100644 --- a/src/sys/nl_ota.c +++ b/src/sys/nl_ota.c @@ -34,22 +34,30 @@ void nl_ota_set_error(const char *e) { s_error = e; } /* Called by the SDK firmware-update path once the image has been written and hashed. sha256 covers the whole payload (T_BOOTER header + compressed image), tail is the - 512 byte end block that used to carry the QConnex RSA signature. */ + 512 byte end block that used to carry the QConnex RSA signature. + NB: this runs on the SDK's own fwup_scheduler task (tls_fwup_init(), stack + FWUP_TASK_STK_SIZE*4, ~2 KB) - NOT on the 8 KB nl_ota task stack. Keep this hook minimal: + one short printf, no extra locals (a version buffer used to sit here and was removed). */ bool nl_fwup_verify_tail(const uint8_t sha256[32], const uint8_t *tail, uint32_t tail_len) { nl_trailer_err_t e = nl_trailer_verify(tail, tail_len, NINLIGHT_DEVICE_ID, sha256); - if (e != NL_TRAILER_OK) { + if (e != NL_TRAILER_OK) s_error = nl_trailer_err_name(e); - printf("ota: trailer rejected (%s)\r\n", s_error); - return false; - } - { - char v[NL_TRAILER_VERSION_LEN]; - nl_trailer_version(tail, v); - printf("ota: trailer ok, incoming version %s\r\n", v); - } - return true; + printf("ota: trailer %s\r\n", e == NL_TRAILER_OK ? "ok" : s_error); + return e == NL_TRAILER_OK; +} + +/* Logs the URL without any "user:pass@" userinfo, so credentials embedded in the URL never + reach the console. s_url is validated by nl_ota_start() to start with "http://". */ +static void log_url(const char *url) +{ + const char *at = strchr(url + 7, '@'); + + if (at != NULL) + printf("ota: downloading http://%s\r\n", at + 1); + else + printf("ota: downloading %s\r\n", url); } static void ota_task(void *arg) @@ -57,7 +65,7 @@ static void ota_task(void *arg) int rc; (void)arg; - printf("ota: downloading %s\r\n", s_url); + log_url(s_url); rc = t_http_fwup(s_url); /* Only reached on failure - the SDK resets the chip itself after a good image. */ if (s_error == NULL) @@ -82,7 +90,7 @@ int nl_ota_start(const char *url) if (url == NULL || strncmp(url, "http://", 7) != 0) return -1; n = strlen(url); - if (n == 0 || n >= sizeof s_url) + if (n <= 7 || n >= sizeof s_url) /* n == 7 is bare "http://" with no host */ return -1; if (s_busy) { s_error = "busy"; @@ -105,6 +113,14 @@ int nl_ota_start(const char *url) if (tls_os_task_create(NULL, "nl_ota", ota_task, NULL, (u8 *)s_stack, sizeof(s_stack), NL_OTA_PRIO, 0) != TLS_OS_SUCCESS) { + /* The task never ran, so clear the ota_pending flag we just persisted above - otherwise + * the next boot wrongly logs "boot after ota attempt" for an update that never started. */ + nl_settings_t *s; + nl_app_lock(); + s = nl_app_settings(); + s->ota_pending = 0; + nl_app_unlock(); + (void)nl_app_save_settings(); s_busy = false; s_error = "busy"; return -1; diff --git a/test/test_api.c b/test/test_api.c index cf79187..3ff2981 100644 --- a/test/test_api.c +++ b/test/test_api.c @@ -323,8 +323,8 @@ static void test_commands(void) TEST_ASSERT_EQUAL_INT(1, fake_factory_calls); call(NL_HTTP_PUT, "/api/v1/system/command", TOKEN, "{\"command\":{\"type\":\"fw_update\",\"url\":\"http://x/y.img\"}}"); - TEST_ASSERT_EQUAL_INT(501, resp.status); - TEST_ASSERT_EQUAL_STRING("fw_update not available", error_text()); + TEST_ASSERT_EQUAL_INT(409, resp.status); + TEST_ASSERT_EQUAL_STRING("fw_update busy or invalid url", error_text()); TEST_ASSERT_EQUAL_STRING("http://x/y.img", fake_fw_url); fake_fw_result = 0; call(NL_HTTP_PUT, "/api/v1/system/command", TOKEN, "{\"command\":{\"type\":\"fw_update\",\"url\":\"http://x/y.img\"}}");