| Message ID | 20230122213659.55793-1-dev@aboehler.at |
|---|---|
| State | Changes Requested |
| Delegated to: | Sander Vanheule |
| Headers | show |
| Series | firmware-utils: tplink-safeloader: Add alternative partition table offset | expand |
Hi Andreas, On Sun, 2023-01-22 at 22:36 +0100, Andreas Böhler wrote: > Some newer OEM firmware files, even for existing devices, have the > fwup-ptn at offset 0x1050 instead of 0x1014. If the fwup-ptn header is not > found at 0x1014, the alternative offset at 0x1050 is tried. > > Signed-off-by: Andreas Böhler <dev@aboehler.at> Would've been handy if you had mentioned some examples. Via the forum I found TL-WPA8631P V3 and TL-WPA8630 V2: https://www.tp-link.com/support/download/tl-wpa8631p/v3.8/#Firmware https://www.tp-link.com/support/download/tl-wpa8630/v2/#Firmware These images have a very suspicious looking '?NEW' at position 0x14, right where the old header used to end. Can this be used to discriminate between the two formats instead? The patch seems to only implement the new format for image conversion to sysupgrade files (-z), but that isn't mentioned in the commit message either. I think it's best if the other input (-x and -i) options would also support the new format. I also have the impression that the file size (minus 0x14 header bytes) is now at position 0x10 instead of 0x0. (Did they swap the MD5 checksum and image size?) No idea what the new header actually contains though. If these devices will still accept the old safeloader format, that probably doesn't need to be implemented yet. > --- > src/tplink-safeloader.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/src/tplink-safeloader.c b/src/tplink-safeloader.c > index ddb5dff..5bf62f3 100644 > --- a/src/tplink-safeloader.c > +++ b/src/tplink-safeloader.c > @@ -3987,6 +3987,7 @@ static void convert_firmware(const char *input, const > char *output) > struct flash_partition_entry *flash_os_image = NULL, > *flash_file_system = NULL; > struct flash_partition_entry *fwup_partition_table = NULL; > size_t firmware_offset = 0x1014; > + size_t firmware_offset_alt = 0x1050; The 0x1014 offset is used in multiple locations, so it would be good to have this as a #define, or a few defines. A more verbose option might be: #define SAFELOADER_HEADER_LEN 0x14 #define SAFELOADER_PAYLOAD_START (SAFELOADER_HEADER_LEN + 0x1000) With matching #define-s for the new format: #define SAFELOADER_HEADER_NEW_LEN 0x3C #define SAFELOADER_PAYLOAD_NEW_START (SAFELOADER_HEADER_LEN + SAFELOADER_HEADER_NEW_LEN + 0x1000) > FILE *input_file, *output_file; > > struct stat statbuf; > @@ -4005,7 +4006,12 @@ static void convert_firmware(const char *input, const > char *output) > error(1, 0, "Can not open output firmware %s", output); > > if (read_partition_table(input_file, firmware_offset, fwup, > MAX_PARTITIONS, 0) != 0) { > - error(1, 0, "Error can not read the partition table (fwup- > ptn)"); > + fprintf(stderr, "DEBUG: can not find partition table at 0x%lx, > trying alternative location at 0x%lx\n", > + firmware_offset, firmware_offset_alt); You could probably drop this heuristic message if you check for '?NEW', and fall back to the old format if that's missing. Best, Sander > + firmware_offset = firmware_offset_alt; > + if (read_partition_table(input_file, firmware_offset, fwup, > MAX_PARTITIONS, 0) != 0) { > + error(1, 0, "Error can not read the partition table > (fwup-ptn)"); > + } > } > > fwup_os_image = find_partition(fwup, MAX_PARTITIONS,
Hi Sander, thanks for the review. On 28/01/2023 16:42, Sander Vanheule wrote: > Hi Andreas, > > On Sun, 2023-01-22 at 22:36 +0100, Andreas Böhler wrote: >> Some newer OEM firmware files, even for existing devices, have the >> fwup-ptn at offset 0x1050 instead of 0x1014. If the fwup-ptn header is not >> found at 0x1014, the alternative offset at 0x1050 is tried. >> >> Signed-off-by: Andreas Böhler <dev@aboehler.at> > Would've been handy if you had mentioned some examples. > Via the forum I found TL-WPA8631P V3 and TL-WPA8630 V2: > > https://www.tp-link.com/support/download/tl-wpa8631p/v3.8/#Firmware > https://www.tp-link.com/support/download/tl-wpa8630/v2/#Firmware > > These images have a very suspicious looking '?NEW' at position 0x14, right where > the old header used to end. Can this be used to discriminate between the two > formats instead? > > The patch seems to only implement the new format for image conversion to > sysupgrade files (-z), but that isn't mentioned in the commit message either. I > think it's best if the other input (-x and -i) options would also support the > new format. Unfortunately, I don't have access to one of the affected devices. I just fixed the "-z" for a forum user and got confirmation that it worked for him. I will try my best to fix it for "-x" and "-i", but I have no clue on the changed (?) header format either. Regards, Andreas
Hi Andreas, On Tue, 2023-01-31 at 12:20 +0100, Andreas Böhler wrote: > Hi Sander, > > thanks for the review. > > > On 28/01/2023 16:42, Sander Vanheule wrote: > > Hi Andreas, > > > > On Sun, 2023-01-22 at 22:36 +0100, Andreas Böhler wrote: > > > Some newer OEM firmware files, even for existing devices, have the > > > fwup-ptn at offset 0x1050 instead of 0x1014. If the fwup-ptn header is not > > > found at 0x1014, the alternative offset at 0x1050 is tried. > > > > > > Signed-off-by: Andreas Böhler <dev@aboehler.at> > > Would've been handy if you had mentioned some examples. > > Via the forum I found TL-WPA8631P V3 and TL-WPA8630 V2: > > > > https://www.tp-link.com/support/download/tl-wpa8631p/v3.8/#Firmware > > https://www.tp-link.com/support/download/tl-wpa8630/v2/#Firmware > > > > These images have a very suspicious looking '?NEW' at position 0x14, right > > where > > the old header used to end. Can this be used to discriminate between the two > > formats instead? > > > > The patch seems to only implement the new format for image conversion to > > sysupgrade files (-z), but that isn't mentioned in the commit message > > either. I > > think it's best if the other input (-x and -i) options would also support > > the > > new format. > > Unfortunately, I don't have access to one of the affected devices. I > just fixed the "-z" for a forum user and got confirmation that it worked > for him. I will try my best to fix it for "-x" and "-i", but I have no > clue on the changed (?) header format either. I've just had a quick look at the "?NEW" header format in a few images, so the remarks in my review were only based on that. Let me have a look this week to see if I can these merge the different tool options a bit. Perhaps it'll be easier to add alternative header formats then. Best, Sander
diff --git a/src/tplink-safeloader.c b/src/tplink-safeloader.c index ddb5dff..5bf62f3 100644 --- a/src/tplink-safeloader.c +++ b/src/tplink-safeloader.c @@ -3987,6 +3987,7 @@ static void convert_firmware(const char *input, const char *output) struct flash_partition_entry *flash_os_image = NULL, *flash_file_system = NULL; struct flash_partition_entry *fwup_partition_table = NULL; size_t firmware_offset = 0x1014; + size_t firmware_offset_alt = 0x1050; FILE *input_file, *output_file; struct stat statbuf; @@ -4005,7 +4006,12 @@ static void convert_firmware(const char *input, const char *output) error(1, 0, "Can not open output firmware %s", output); if (read_partition_table(input_file, firmware_offset, fwup, MAX_PARTITIONS, 0) != 0) { - error(1, 0, "Error can not read the partition table (fwup-ptn)"); + fprintf(stderr, "DEBUG: can not find partition table at 0x%lx, trying alternative location at 0x%lx\n", + firmware_offset, firmware_offset_alt); + firmware_offset = firmware_offset_alt; + if (read_partition_table(input_file, firmware_offset, fwup, MAX_PARTITIONS, 0) != 0) { + error(1, 0, "Error can not read the partition table (fwup-ptn)"); + } } fwup_os_image = find_partition(fwup, MAX_PARTITIONS,
Some newer OEM firmware files, even for existing devices, have the fwup-ptn at offset 0x1050 instead of 0x1014. If the fwup-ptn header is not found at 0x1014, the alternative offset at 0x1050 is tried. Signed-off-by: Andreas Böhler <dev@aboehler.at> --- src/tplink-safeloader.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)