From 5c8e715e7eb8e451ab2e5ec7ef51394f14123760 Mon Sep 17 00:00:00 2001 From: AI Dev Date: Wed, 14 Oct 2026 13:55:49 +0000 Subject: [PATCH 1/2] upgrade: move cameras on OpenIPC's own bootloader, in the memory they have Run against a camera on OpenIPC's bootloader rather than a vendor one, the layout move refused, and on a lite image it would not have fitted in memory anyway: - OpenIPC's bootcmd reads the kernel to a variable, "sf read ${baseaddr} 0x50000 0x200000". The edit required a number for the address and refused. The address may now be any word; only the offset and the size change. - Its default bootargs say mtdparts=${mtdparts}. That word is now left as it is, and the mtdparts variable is set instead, its mtd id read from it. A literal mtdparts= is still edited in place, and the variable, where there is one, follows either way. - bootcmdnor, the copy of the kernel read that its setnor8m/setnor16m and failsafe run, gets the same edit. A bundle may not set it or mtdparts. - Each image was copied into a buffer the size of its partition: 13 MB for an ultimate bundle, on top of the bundle itself, which already sits in RAM when it is in /tmp. A lite camera booted with mem=32M has about 16 MB. Images are now written from the mapped bundle, and the end of each is padded in one erase block. The settings partition keeps only the carried image, not 2.7 MB of 0xFF. QEMU (hi3516ev300) with the openipc.org 16 MB full image, OpenIPC's bootloader, and a saved environment in the shape it keeps (mtdparts=${mtdparts}, sf read ${baseaddr}): - generic lite moved from the 16 MB layout to the 8 MB one; - then generic ultimate moved back to the 16 MB layout; - the carried settings survived both moves; - mtdparts, bootcmd and bootcmdnor followed each time. --- README.md | 9 ++- src/backup.c | 126 +++++++++++++++++++++++++++++----------- src/upgrade_env.c | 34 ++++++++--- src/upgrade_env.h | 26 ++++++--- src/upgrade_env_test.c | 37 +++++++++++- tools/upgrade_bundle.py | 2 +- 6 files changed, 180 insertions(+), 54 deletions(-) diff --git a/README.md b/README.md index 5f0e153..b677e37 100644 --- a/README.md +++ b/README.md @@ -308,7 +308,10 @@ What a `--layout` bundle does on the camera: there (`wtf` on XiongMai); * edits `bootargs` and `bootcmd` rather than replacing them: only `root=`, `mtdparts=` and the size of the kernel read change, so a vendor - bootloader's other arguments survive; + bootloader's other arguments survive. With OpenIPC's own bootloader the + partitions live in a `mtdparts` variable that `bootargs` refers to + (`mtdparts=${mtdparts}`), and the kernel read in `bootcmdnor` as well: + those are edited the same way; * carries the settings: the jffs2 overlay is copied, once it is read-only, into a jffs2 image written where the new `rootfs_data` starts. The camera comes back with its passwords, keys and configuration. `--wipe-settings` @@ -316,6 +319,10 @@ What a `--layout` bundle does on the camera: * sets the U-Boot variables given with `--env`, such as the address the new firmware upgrades itself from. +The images are written straight from the bundle, so the bundle itself is +the memory it needs: put it in `/tmp` (a 10 MB ultimate bundle wants that +much free there), or on an SD card. + Everything that can be refused is refused before any process is stopped: an image larger than its partition, settings that will not fit the new `rootfs_data`, a bootloader environment with no literal `mtdparts=` or no diff --git a/src/backup.c b/src/backup.c index 23afd80..e7a5f4c 100644 --- a/src/backup.c +++ b/src/backup.c @@ -225,6 +225,11 @@ typedef struct { char sha1[9]; char name[64]; char *data; + // When set, only the first `payload` bytes are in `data` and the rest of + // `size` is erased flash (0xFF): an image is written from where it lies + // rather than copied into a partition-sized buffer. + bool padded; + size_t payload; bool is_ubi; int ubi_device; int vol_id; @@ -913,6 +918,19 @@ static bool do_flash(const char *phase, stored_mtd_t *mtdbackup, printf("%s %s\n", phase, mtdbackup[i].name); size_t chunk = mtd->erasesize; + // One block to pad the end of an image in, allocated on the trial + // pass, before anything is locked in memory + static char *pad; + static size_t padlen; + if (padlen < chunk) { + free(pad); + pad = malloc(chunk); + padlen = pad ? chunk : 0; + if (!pad) { + fprintf(stderr, "Allocation error\n"); + return false; + } + } int cnt = mtdbackup[i].size / chunk; for (int c = 0; c < cnt; c++) { size_t this_offset; @@ -935,8 +953,19 @@ static bool do_flash(const char *phase, stored_mtd_t *mtdbackup, this_offset, mtd->erasesize, mtdbackup[i].data + c * chunk, chunk); #else - if (!mtd_write(newi, this_offset, mtd->erasesize, - mtdbackup[i].data + c * chunk, chunk)) { + const char *src = mtdbackup[i].data + c * chunk; + if (mtdbackup[i].padded && + (c + 1) * chunk > mtdbackup[i].payload) { + size_t have = c * chunk < mtdbackup[i].payload + ? mtdbackup[i].payload - c * chunk + : 0; + memset(pad, 0xff, chunk); + if (have) + memcpy(pad, src, have); + src = pad; + } + if (!mtd_write(newi, this_offset, mtd->erasesize, src, + chunk)) { fprintf(stderr, "\nSomething went wrong, aborting...\n"); return false; @@ -1205,8 +1234,8 @@ static void skip_signals() { * jffs2 image written there along with the kernel and the rootfs. */ typedef struct { - char dir[80]; // where the old settings partition is mounted - char *region; // the new settings partition's contents, flash-sized + char dir[80]; // where the old settings partition is mounted + stored_mtd_t *region; // the new settings partition, as written size_t region_len, erasesize; } carry_t; @@ -1256,12 +1285,22 @@ static bool build_carry(void *arg) { jffs2_img_t *img = jffs2_new(c->erasesize, c->region_len - headroom); bool ok = img && jffs2_add_tree(img, c->dir, "work"); if (ok) { + // Only the image is kept; the rest of the partition is written as + // erased flash size_t len; const uint8_t *data = jffs2_data(img, &len); - memset(c->region, 0xff, c->region_len); - memcpy(c->region, data, len); - printf("Settings: %d entries in %zu of %zu KB\n", jffs2_entries(img), - len / 1024, c->region_len / 1024); + char *copy = malloc(len); + if (copy) { + memcpy(copy, data, len); + free(c->region->data); + c->region->data = copy; + c->region->payload = len; + printf("Settings: %d entries in %zu of %zu KB\n", + jffs2_entries(img), len / 1024, c->region_len / 1024); + } else { + fprintf(stderr, "Cannot carry the settings: out of memory\n"); + ok = false; + } } else fprintf(stderr, "Cannot carry the settings: %s\n", img ? jffs2_error(img) : "out of memory"); @@ -1447,11 +1486,19 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { } bool keep_front = !writes_boot && !mtdparts_pr && !skip_envs && !offset; if (keep_front) { + // The partitions are named in bootargs, or, as OpenIPC's own + // bootloader keeps them, in a variable bootargs refers to char id[64]; - if (!upgrade_mtd_id(uboot_env_get_param("bootargs"), id, sizeof(id))) { - fprintf(stderr, "The bootloader's bootargs name no partitions " - "(no literal mtdparts=), so there is no telling " - "what to keep. Nothing was written.\n"); + const char *ba = uboot_env_get_param("bootargs"); + if (!upgrade_mtd_id(ba, id, sizeof(id)) && + !(upgrade_bootargs_mtdparts_is_var(ba) && + upgrade_mtd_id_value(uboot_env_get_param("mtdparts"), id, + sizeof(id)))) { + fprintf(stderr, "The bootloader's environment names no " + "partitions (no literal mtdparts= in bootargs or " + "in a mtdparts variable it refers to), so there " + "is no telling what to keep. Nothing was " + "written.\n"); ret = 1; goto bailout; } @@ -1517,15 +1564,13 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { ret = 1; goto bailout; } + // Written from the mapped bundle: a copy padded to the partition + // would be 13 MB more for an ultimate image, on a camera that may + // have 16 MB available all told mtdwrite[i].size = ceil_up(payload, psize ? psize : mtd.erasesize); - mtdwrite[i].data = malloc(mtdwrite[i].size); - if (mtdwrite[i].data == NULL) { - fprintf(stderr, "Allocation error\n"); - return 1; - } - memcpy(mtdwrite[i].data, read_ptr, payload); - memset(mtdwrite[i].data + payload, 0xff, - mtdwrite[i].size - payload); + mtdwrite[i].data = read_ptr; + mtdwrite[i].padded = true; + mtdwrite[i].payload = payload; read_ptr += payload; } @@ -1584,7 +1629,7 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { i++; } - munmap(jsond, len); + // The bundle stays mapped: the images are written from it if (root_part == -1 || kernel_part == -1) { fprintf(stderr, @@ -1596,7 +1641,7 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { ",-(rootfs_data)"); carry_t carry = {.erasesize = mtd.erasesize}; - char new_bootargs[1024], new_bootcmd[1024]; + char new_bootargs[1024], new_bootcmd[1024], new_bootcmdnor[1024] = ""; if (keep_front) { if ((ssize_t)goff >= mtd.totalsz) { fprintf(stderr, @@ -1610,13 +1655,11 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { // what lies there now is the middle of the old one, and jffs2 would // mount its stale nodes as files carry.region_len = mtd.totalsz - goff; - carry.region = malloc(carry.region_len); - if (!carry.region) { - fprintf(stderr, "Allocation error\n"); - ret = 1; - goto bailout; - } - memset(carry.region, 0xff, carry.region_len); + carry.region = &mtdwrite[i]; + strcpy(mtdwrite[i].name, "rootfs_data"); + mtdwrite[i].off_flashb = goff; + mtdwrite[i].size = carry.region_len; + mtdwrite[i].padded = true; // erased unless settings are carried int data_part = -1; for (int p = 0; p < MAX_MTDBLOCKS && data_part < 0; p++) if (!strcmp(mtd.part[p].name, "rootfs_data")) @@ -1645,10 +1688,6 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { ret = 5; goto bailout; } - strcpy(mtdwrite[i].name, "rootfs_data"); - mtdwrite[i].off_flashb = goff; - mtdwrite[i].size = carry.region_len; - mtdwrite[i].data = carry.region; i++; if (!upgrade_bootargs(uboot_env_get_param("bootargs"), root_part, @@ -1664,8 +1703,19 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { ret = 1; goto bailout; } + // OpenIPC's bootloader keeps a second copy of the kernel read, which + // its setnor8m/setnor16m and failsafe run; it follows the layout too + const char *nor = uboot_env_get_param("bootcmdnor"); + if (nor && !upgrade_bootcmd(nor, mtdwrite[kernel_part].off_flashb, + mtdwrite[kernel_part].size, new_bootcmdnor, + sizeof(new_bootcmdnor))) + printf("bootcmdnor has no kernel read to edit; left as it is\n"); printf("bootargs will be: %s\nbootcmd will be: %s\n", new_bootargs, new_bootcmd); + if (uboot_env_get_param("mtdparts")) + printf("mtdparts will be: %s\n", mtdparts); + if (*new_bootcmdnor) + printf("bootcmdnor will be: %s\n", new_bootcmdnor); } // The bundle's variables, checked while nothing is stopped. bootargs and @@ -1681,7 +1731,9 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { } cJSON_ArrayForEach(jvar, jenv) { if (!cJSON_IsString(jvar) || !strcmp(jvar->string, "bootargs") || - !strcmp(jvar->string, "bootcmd")) { + !strcmp(jvar->string, "bootcmd") || + !strcmp(jvar->string, "bootcmdnor") || + !strcmp(jvar->string, "mtdparts")) { fprintf(stderr, "The bundle sets '%s', which upgrade %s. Nothing was " "written.\n", @@ -1714,6 +1766,12 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { } if (keep_front) { + // The variable a bootargs of mtdparts=${mtdparts} reads, and the + // copy setnor8m/setnor16m keep, go with the layout either way + if (uboot_env_get_param("mtdparts")) + set_env_param_ram("mtdparts", mtdparts); + if (*new_bootcmdnor) + set_env_param_ram("bootcmdnor", new_bootcmdnor); set_env_param_ram("bootcmd", new_bootcmd); set_env_param_rom("bootargs", new_bootargs, mtd.env_dev, mtd.env_offset, mtd.erasesize); diff --git a/src/upgrade_env.c b/src/upgrade_env.c index ee134bc..20eb30a 100644 --- a/src/upgrade_env.c +++ b/src/upgrade_env.c @@ -15,6 +15,22 @@ static const char *find_word(const char *s, const char *key) { static size_t word_len(const char *w) { return strcspn(w, " "); } +bool upgrade_mtd_id_value(const char *w, char *id, size_t idsz) { + if (!w) + return false; + size_t n = strcspn(w, ": "); + if (!n || w[n] != ':' || memchr(w, '$', n) || n >= idsz) + return false; + memcpy(id, w, n); + id[n] = '\0'; + return true; +} + +bool upgrade_bootargs_mtdparts_is_var(const char *bootargs) { + const char *w = bootargs ? find_word(bootargs, "mtdparts=") : NULL; + return w && w[strlen("mtdparts=")] == '$'; +} + bool upgrade_mtd_id(const char *bootargs, char *id, size_t idsz) { const char *w = bootargs ? find_word(bootargs, "mtdparts=") : NULL; if (!w) @@ -31,8 +47,9 @@ bool upgrade_mtd_id(const char *bootargs, char *id, size_t idsz) { bool upgrade_bootargs(const char *old, int root_idx, const char *mtdparts, char *out, size_t outsz) { char id[64]; - if (!old || !upgrade_mtd_id(old, id, sizeof(id)) || - !find_word(old, "root=")) + bool var = upgrade_bootargs_mtdparts_is_var(old); + if (!old || !find_word(old, "root=") || + (!var && !upgrade_mtd_id(old, id, sizeof(id)))) return false; size_t used = 0; @@ -49,7 +66,7 @@ bool upgrade_bootargs(const char *old, int root_idx, const char *mtdparts, if (!strncmp(p, "root=", 5)) w = snprintf(out + used, outsz - used, "root=/dev/mtdblock%d", root_idx); - else if (!strncmp(p, "mtdparts=", 9)) + else if (!strncmp(p, "mtdparts=", 9) && !var) w = snprintf(out + used, outsz - used, "mtdparts=%s", mtdparts); else w = snprintf(out + used, outsz - used, "%.*s", (int)n, p); @@ -70,13 +87,14 @@ bool upgrade_bootcmd(const char *old, uint32_t off, uint32_t size, char *out, if (!rd || strstr(rd + 1, "sf read ")) return false; + // The load address stays, a number or a variable const char *p = rd + strlen("sf read "); - char *end; - strtoul(p, &end, 16); // the load address stays - if (end == p || *end != ' ') + size_t alen = strcspn(p, " ;"); + if (!alen || p[alen] != ' ') return false; - const char *addr_end = end; - p = end + 1; + const char *addr_end = p + alen; + char *end; + p = addr_end + 1; strtoul(p, &end, 16); if (end == p || *end != ' ') return false; diff --git a/src/upgrade_env.h b/src/upgrade_env.h index 7a83895..dc2dff0 100644 --- a/src/upgrade_env.h +++ b/src/upgrade_env.h @@ -18,17 +18,29 @@ * mtdparts= (one that names a variable, "${mtdparts}", is not literal). */ bool upgrade_mtd_id(const char *bootargs, char *id, size_t idsz); -/* bootargs with root= pointing at /dev/mtdblock and mtdparts= - * replaced by `mtdparts` (id included), every other word kept in its place. - * False, leaving `out` unspecified, when either word is missing or the - * mtdparts= value is not literal: an edit that cannot find what it edits - * must not produce a line that boots the old layout. */ +/* The same from the value of OpenIPC's own `mtdparts` variable, + * "hi_sfc:256k(boot),...", which its bootloader's default bootargs refer to + * as mtdparts=${mtdparts}. */ +bool upgrade_mtd_id_value(const char *mtdparts, char *id, size_t idsz); + +/* Whether bootargs takes its partitions from a variable (mtdparts=${...}), + * in which case that variable is what has to change. */ +bool upgrade_bootargs_mtdparts_is_var(const char *bootargs); + +/* bootargs with root= pointing at /dev/mtdblock and a literal + * mtdparts= replaced by `mtdparts` (id included), every other word kept in + * its place. A mtdparts= that names a variable is left as it is: the + * variable is set instead. False, leaving `out` unspecified, when either + * word is missing: an edit that cannot find what it edits must not produce + * a line that boots the old layout. */ bool upgrade_bootargs(const char *old, int root_idx, const char *mtdparts, char *out, size_t outsz); /* bootcmd with the kernel read, "sf read ", reading - * `size` bytes from `off` instead. False when there is no such read, or more - * than one. */ + * `size` bytes from `off` instead. The address may be a variable, as + * OpenIPC's bootloader writes it ("sf read ${baseaddr} 0x50000 0x200000"); + * the offset and size are numbers. False when there is no such read, or + * more than one. */ bool upgrade_bootcmd(const char *old, uint32_t off, uint32_t size, char *out, size_t outsz); diff --git a/src/upgrade_env_test.c b/src/upgrade_env_test.c index 271c9a9..3bd3775 100644 --- a/src/upgrade_env_test.c +++ b/src/upgrade_env_test.c @@ -64,9 +64,10 @@ int main(void) { CHECK(!upgrade_bootargs("console=ttyAMA0 mtdparts=sfc:1k(a)", 3, "sfc:2k(a)", out, sizeof(out)), "a line without root= edited"); - CHECK(!upgrade_bootargs("root=/dev/mtdblock3 mtdparts=${mtdparts}", 3, - "sfc:2k(a)", out, sizeof(out)), - "a variable mtdparts edited"); + CHECK(upgrade_bootargs("root=/dev/mtdblock3 mtdparts=${mtdparts}", 4, + "sfc:2k(a)", out, sizeof(out)) && + !strcmp(out, "root=/dev/mtdblock4 mtdparts=${mtdparts}"), + "a variable mtdparts was not left for its variable: '%s'", out); CHECK(!upgrade_bootargs(XM_BOOTARGS, 3, "hi_sfc:1k(a)", out, 32), "a line that does not fit returned as done"); CHECK(!upgrade_bootargs(NULL, 3, "hi_sfc:1k(a)", out, sizeof(out)), @@ -89,6 +90,36 @@ int main(void) { out, sizeof(out)), "a read without a size edited"); + // OpenIPC's own bootloader, as a camera flashed with it keeps its + // environment: the load address is a variable, and the partitions are + // also kept in a variable of their own. + const char *OI_BOOTCMD = + "setenv setargs setenv bootargs ${bootargs}; run setargs; sf probe 0; " + "sf read ${baseaddr} 0x50000 0x200000; bootm ${baseaddr}"; + CHECK(upgrade_bootcmd(OI_BOOTCMD, 0x50000, 0x300000, out, sizeof(out)) && + !strcmp(out, "setenv setargs setenv bootargs ${bootargs}; run " + "setargs; sf probe 0; sf read ${baseaddr} 0x50000 " + "0x300000; bootm ${baseaddr}"), + "variable load address: '%s'", out); + CHECK(!upgrade_bootcmd("sf read ${baseaddr}; bootm", 0x50000, 0x300000, + out, sizeof(out)), + "a read with no offset edited"); + + CHECK(upgrade_mtd_id_value("hi_sfc:256k(boot),64k(env)", id, sizeof(id)) && + !strcmp(id, "hi_sfc"), + "mtd id from the variable '%s'", id); + // Its default bootargs name the variable: root= moves, mtdparts= stays + // and the variable is what changes + const char *OI_DEFAULT = "mem=${osmem} console=ttyAMA0,115200 panic=20 " + "root=/dev/mtdblock3 rootfstype=squashfs " + "init=/init mtdparts=${mtdparts} ${extras}"; + CHECK(upgrade_bootargs_mtdparts_is_var(OI_DEFAULT), "variable not seen"); + CHECK(!upgrade_bootargs_mtdparts_is_var(XM_BOOTARGS), + "a literal taken for a variable"); + CHECK(upgrade_bootargs(OI_DEFAULT, 3, "sfc:9k(x)", out, sizeof(out)) && + !strcmp(out, OI_DEFAULT), + "a variable mtdparts= was replaced: '%s'", out); + if (!failed) puts("upgrade_env_test: OK"); return failed; diff --git a/tools/upgrade_bundle.py b/tools/upgrade_bundle.py index 853360e..b6f7745 100755 --- a/tools/upgrade_bundle.py +++ b/tools/upgrade_bundle.py @@ -159,7 +159,7 @@ def main(): key, sep, value = kv.partition('=') if not sep or not key: parser.error(f'--env {kv}: expected KEY=VALUE') - if key in ('bootargs', 'bootcmd'): + if key in ('bootargs', 'bootcmd', 'bootcmdnor', 'mtdparts'): parser.error(f'--env {key}: ipctool writes it for the new ' 'layout itself') env[key] = value From 155300dd8997de1c86dbb528e5e334cdaa2ef0a8 Mon Sep 17 00:00:00 2001 From: AI Dev Date: Wed, 14 Oct 2026 14:45:12 +0000 Subject: [PATCH 2/2] upgrade: refuse a bootcmdnor or mtdparts reference it cannot follow Four findings from review: - A bootcmdnor with a kernel read that could not be edited was left as it was, so failsafe and setnor8m/16m would have read the old kernel's place. A copy with no read is still left alone. One whose read cannot be edited, more than one read, or an edit that does not fit its buffer (which used to be saved truncated) is now refused before anything is stopped, like bootcmd. - bootargs naming any variable as mtdparts= was taken as a reference to the mtdparts variable, which is the only one upgrade sets. A ${mtdpartsnor} would have moved root= and left the kernel the old partitions. Only ${mtdparts} and $mtdparts count now; any other reference is neither literal nor followable, and is refused. - With --wipe-settings, the settings partition has no data, and each block formed an offset from a null pointer before the padding buffer was chosen. The pointer is now formed only where there are bytes behind it. QEMU (hi3516ev300): - --wipe-settings on OpenIPC's bootloader moved to the 8 MB layout and booted, with the kernel formatting the fresh settings partition; - the XiongMai camera's move to ultimate still carries every setting. --- src/backup.c | 38 ++++++++++++++++++++++++++++---------- src/upgrade_env.c | 9 ++++++++- src/upgrade_env.h | 6 ++++-- src/upgrade_env_test.c | 10 ++++++++++ 4 files changed, 50 insertions(+), 13 deletions(-) diff --git a/src/backup.c b/src/backup.c index e7a5f4c..9462b5e 100644 --- a/src/backup.c +++ b/src/backup.c @@ -953,17 +953,21 @@ static bool do_flash(const char *phase, stored_mtd_t *mtdbackup, this_offset, mtd->erasesize, mtdbackup[i].data + c * chunk, chunk); #else - const char *src = mtdbackup[i].data + c * chunk; + // A pointer into the data only where there is data: the + // settings partition of --wipe-settings has none at all + const char *src; + size_t off = c * chunk; if (mtdbackup[i].padded && - (c + 1) * chunk > mtdbackup[i].payload) { - size_t have = c * chunk < mtdbackup[i].payload - ? mtdbackup[i].payload - c * chunk + off + chunk > mtdbackup[i].payload) { + size_t have = off < mtdbackup[i].payload + ? mtdbackup[i].payload - off : 0; memset(pad, 0xff, chunk); if (have) - memcpy(pad, src, have); + memcpy(pad, mtdbackup[i].data + off, have); src = pad; - } + } else + src = mtdbackup[i].data + off; if (!mtd_write(newi, this_offset, mtd->erasesize, src, chunk)) { fprintf(stderr, @@ -1705,11 +1709,25 @@ static int do_upgrade(const char *filename, bool force, bool wipe_settings) { } // OpenIPC's bootloader keeps a second copy of the kernel read, which // its setnor8m/setnor16m and failsafe run; it follows the layout too + // A copy with no kernel read is left alone; one with a read that + // cannot be edited is refused like bootcmd, or failsafe would boot + // the old kernel's place const char *nor = uboot_env_get_param("bootcmdnor"); - if (nor && !upgrade_bootcmd(nor, mtdwrite[kernel_part].off_flashb, - mtdwrite[kernel_part].size, new_bootcmdnor, - sizeof(new_bootcmdnor))) - printf("bootcmdnor has no kernel read to edit; left as it is\n"); + bool nor_edited = false; + if (nor && strstr(nor, "sf read")) { + nor_edited = upgrade_bootcmd(nor, mtdwrite[kernel_part].off_flashb, + mtdwrite[kernel_part].size, + new_bootcmdnor, + sizeof(new_bootcmdnor)); + if (!nor_edited) { + fprintf(stderr, "bootcmdnor reads the kernel in a way this " + "cannot edit. Nothing was written.\n"); + ret = 1; + goto bailout; + } + } + if (!nor_edited) + new_bootcmdnor[0] = '\0'; printf("bootargs will be: %s\nbootcmd will be: %s\n", new_bootargs, new_bootcmd); if (uboot_env_get_param("mtdparts")) diff --git a/src/upgrade_env.c b/src/upgrade_env.c index 20eb30a..1e8791c 100644 --- a/src/upgrade_env.c +++ b/src/upgrade_env.c @@ -28,7 +28,14 @@ bool upgrade_mtd_id_value(const char *w, char *id, size_t idsz) { bool upgrade_bootargs_mtdparts_is_var(const char *bootargs) { const char *w = bootargs ? find_word(bootargs, "mtdparts=") : NULL; - return w && w[strlen("mtdparts=")] == '$'; + if (!w) + return false; + // Only the variable upgrade sets: a reference to any other would leave + // the kernel the old partitions while root= moved on + w += strlen("mtdparts="); + size_t n = word_len(w); + return (n == 11 && !strncmp(w, "${mtdparts}", 11)) || + (n == 9 && !strncmp(w, "$mtdparts", 9)); } bool upgrade_mtd_id(const char *bootargs, char *id, size_t idsz) { diff --git a/src/upgrade_env.h b/src/upgrade_env.h index dc2dff0..a6938d7 100644 --- a/src/upgrade_env.h +++ b/src/upgrade_env.h @@ -23,8 +23,10 @@ bool upgrade_mtd_id(const char *bootargs, char *id, size_t idsz); * as mtdparts=${mtdparts}. */ bool upgrade_mtd_id_value(const char *mtdparts, char *id, size_t idsz); -/* Whether bootargs takes its partitions from a variable (mtdparts=${...}), - * in which case that variable is what has to change. */ +/* Whether bootargs takes its partitions from the `mtdparts` variable + * (mtdparts=${mtdparts} or mtdparts=$mtdparts), in which case that variable + * is what has to change. A reference to any other variable is not this: it + * is neither literal nor the variable upgrade sets, and is refused. */ bool upgrade_bootargs_mtdparts_is_var(const char *bootargs); /* bootargs with root= pointing at /dev/mtdblock and a literal diff --git a/src/upgrade_env_test.c b/src/upgrade_env_test.c index 3bd3775..6df7324 100644 --- a/src/upgrade_env_test.c +++ b/src/upgrade_env_test.c @@ -119,6 +119,16 @@ int main(void) { CHECK(upgrade_bootargs(OI_DEFAULT, 3, "sfc:9k(x)", out, sizeof(out)) && !strcmp(out, OI_DEFAULT), "a variable mtdparts= was replaced: '%s'", out); + CHECK(upgrade_bootargs_mtdparts_is_var("root=/dev/mtdblock3 " + "mtdparts=$mtdparts"), + "$mtdparts not seen"); + // Another variable is neither literal nor the one upgrade sets: refused, + // or root= would move while the kernel kept the old partitions + const char *OTHER = "root=/dev/mtdblock3 mtdparts=${mtdpartsnor} x"; + CHECK(!upgrade_bootargs_mtdparts_is_var(OTHER), + "${mtdpartsnor} taken for ${mtdparts}"); + CHECK(!upgrade_bootargs(OTHER, 3, "sfc:9k(x)", out, sizeof(out)), + "bootargs naming another variable was edited"); if (!failed) puts("upgrade_env_test: OK");