[PATCH] firmware-utils: tplink-safeloader: Add alternative partition table offset

[Sander Vanheule]

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 at 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,