mtd: bcm47xxpart: improve handling TRX partition size
PATCH 1/1 mtd: bcm47xxpart: improve handling TRX partition size
From: Rafał Miłecki <[email protected]>
When bcm47xxpart finds a TRX partition (container) it's supposed to jump to the end of it and keep looking for more partitions. TRX and its subpartitions are handled be a separated parser.
The problem with old code was relying on the length specified in a TRX header. That isn't reliable as TRX is commonly modified to have checksum cover only non-changing subpartitions. Otherwise modifying e.g. a rootfs would result in CRC32 mismatch and bootloader refusing to boot a firmware.
Fix it by trying better to figure out a real TRX size. We can securely assume that TRX has to cover all subpartitions and the last one is at least of a block size in size. Then compare it with a length field.
This makes code more optimal & reliable thanks to skipping data that shouldn't be parsed.
--- drivers/mtd/bcm47xxpart.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)
drivers/mtd/bcm47xxpart.c +18 -4
@@ -186,6 +186,8 @@ static int bcm47xxpart_parse(struct mtd_info *master,/* TRX */ if (buf[0x000 /4 ] ==TRX_MAGIC ) {struct trx_header *trx; +uint32_t last_subpart; +uint32_t trx_size;if (trx_num >=ARRAY_SIZE (trx_parts)) pr_warn("No enough space to store another TRX found at 0x%X\n",
@@ -195,11 +197,23 @@ static int bcm47xxpart_parse(struct mtd_info *master,bcm47xxpart_add_part (&parts[curr_part++],"firmware" , offset,0 ); -/* Jump to the end of TRX */ +/* +* Try to find TRX size. The "length" field isn't fully +* reliable as it could be decreased to make CRC32 cover +* only part of TRX data. It's commonly used as checksum +* can't cover e.g. ever-changing rootfs partition. +* Use offsets as helpers for assuming min TRX size. +*/ trx = (struct trx_header *)buf; - offset =roundup (offset + trx->length, blocksize); -/* Next loop iteration will increase the offset */ - offset -= blocksize; + last_subpart =max3 (trx->offset[0 ], trx->offset[1 ], + trx->offset[2 ]); + trx_size =max (trx->length, last_subpart + blocksize); + +/* +* Skip the TRX data. Decrease offset by block size as +* the next loop iteration will increase it. +*/ + offset +=roundup (trx_size, blocksize) - blocksize;continue ; }
Signature · 1 line
2.12.0
Quoted 5 lines · On Thu, 12 Apr 2018 07:24:52 +0200 Rafał Miłecki <[email protected]> wrote:
From: Rafał Miłecki <[email protected]>
When bcm47xxpart finds a TRX partition (container) it's supposed to jump to the end of it and keep looking for more partitions. TRX and its subpartitions are handled be a separated parser.
^ by a separate parser.
No need to send a new version, I'll fix it when applying.
Quoted 7 lines
The problem with old code was relying on the length specified in a TRX header. That isn't reliable as TRX is commonly modified to have checksum cover only non-changing subpartitions. Otherwise modifying e.g. a rootfs would result in CRC32 mismatch and bootloader refusing to boot a firmware.
Quoted 6 lines
Fix it by trying better to figure out a real TRX size. We can securely assume that TRX has to cover all subpartitions and the last one is at least of a block size in size. Then compare it with a length field.
This makes code more optimal & reliable thanks to skipping data that shouldn't be parsed.
I didn't check the TRX parsing logic, so I'm assuming you know what you do here and you've tested the modifications ;-).
Quoted 47 lines
Signed-off-by Rafał Miłecki <[email protected]>--- drivers/mtd/bcm47xxpart.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)drivers/mtd/bcm47xxpart.c +18
-4@@ -186,6 +186,8 @@ static int bcm47xxpart_parse(struct mtd_info *master,/* TRX */ if (buf[0x000 /4 ] ==TRX_MAGIC ) {struct trx_header *trx; +uint32_t last_subpart; +uint32_t trx_size;if (trx_num >=ARRAY_SIZE (trx_parts)) pr_warn("No enough space to store another TRX found at 0x%X\n",@@ -195,11 +197,23 @@ static int bcm47xxpart_parse(struct mtd_info *master,bcm47xxpart_add_part (&parts[curr_part++],"firmware" , offset,0 ); -/* Jump to the end of TRX */ +/* +* Try to find TRX size. The "length" field isn't fully +* reliable as it could be decreased to make CRC32 cover +* only part of TRX data. It's commonly used as checksum +* can't cover e.g. ever-changing rootfs partition. +* Use offsets as helpers for assuming min TRX size. +*/ trx = (struct trx_header *)buf; - offset =roundup (offset + trx->length, blocksize); -/* Next loop iteration will increase the offset */ - offset -= blocksize; + last_subpart =max3 (trx->offset[0 ], trx->offset[1 ], + trx->offset[2 ]); + trx_size =max (trx->length, last_subpart + blocksize); + +/* +* Skip the TRX data. Decrease offset by block size as +* the next loop iteration will increase it. +*/ + offset +=roundup (trx_size, blocksize) - blocksize;continue ; }
Quoted 5 lines · On Sat, 12 May 2018 09:51:02 +0200 Boris Brezillon…
Quoted 2 lines · On Thu, 12 Apr 2018 07:24:52 +0200 Rafał Miłecki <[email protected]> wrote:
From: Rafał Miłecki <[email protected]>
Quoted 7 lines
Quoted 3 lines
When bcm47xxpart finds a TRX partition (container) it's supposed to jump to the end of it and keep looking for more partitions. TRX and its subpartitions are handled be a separated parser.
^ by a separate parser.
No need to send a new version, I'll fix it when applying.
Applied after fixing the typo.
Thanks,
Boris
Quoted 70 lines
Quoted 8 lines
The problem with old code was relying on the length specified in a TRX header. That isn't reliable as TRX is commonly modified to have checksum cover only non-changing subpartitions. Otherwise modifying e.g. a rootfs would result in CRC32 mismatch and bootloader refusing to boot a firmware.
Quoted 6 lines
Fix it by trying better to figure out a real TRX size. We can securely assume that TRX has to cover all subpartitions and the last one is at least of a block size in size. Then compare it with a length field.
This makes code more optimal & reliable thanks to skipping data that shouldn't be parsed.
I didn't check the TRX parsing logic, so I'm assuming you know what you do here and you've tested the modifications ;-).
Quoted 37 lines
Signed-off-by Rafał Miłecki <[email protected]>--- drivers/mtd/bcm47xxpart.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)drivers/mtd/bcm47xxpart.c +11
-4@@ -186,6 +186,8 @@ static int bcm47xxpart_parse(struct mtd_info *master,/* TRX */ if (buf[0x000 /4 ] ==TRX_MAGIC ) {struct trx_header *trx; +uint32_t last_subpart; +uint32_t trx_size;if (trx_num >=ARRAY_SIZE (trx_parts)) pr_warn("No enough space to store another TRX found at 0x%X\n",@@ -195,11 +197,23 @@ static int bcm47xxpart_parse(struct mtd_info *master,bcm47xxpart_add_part (&parts[curr_part++],"firmware" , offset,0 ); -/* Jump to the end of TRX */ +/* +* Try to find TRX size. The "length" field isn't fully +* reliable as it could be decreased to make CRC32 cover +* only part of TRX data. It's commonly used as checksum +* can't cover e.g. ever-changing rootfs partition. +* Use offsets as helpers for assuming min TRX size. +*/ trx = (struct trx_header *)buf; - offset =roundup (offset + trx->length, blocksize); -/* Next loop iteration will increase the offset */ - offset -= blocksize; + last_subpart =max3 (trx->offset[0 ], trx->offset[1 ], + trx->offset[2 ]);drivers/mtd/bcm47xxpart.c static int bcm47xxpart_parse(struct mtd_info *master, · line 219
Quoted 10 lines
drivers/mtd/bcm47xxpart.c +7
-0+ trx_size =max (trx->length, last_subpart + blocksize); + +/* +* Skip the TRX data. Decrease offset by block size as +* the next loop iteration will increase it. +*/ + offset +=roundup (trx_size, blocksize) - blocksize;continue ; }______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/