| Message ID | 20180412052452.11498-1-zajec5@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | mtd: bcm47xxpart: improve handling TRX partition size | expand |
On Thu, 12 Apr 2018 07:24:52 +0200 Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > 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. > > 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. I didn't check the TRX parsing logic, so I'm assuming you know what you do here and you've tested the modifications ;-). > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > drivers/mtd/bcm47xxpart.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c > index fe2581d9d882..1f0239848ebe 100644 > --- a/drivers/mtd/bcm47xxpart.c > +++ b/drivers/mtd/bcm47xxpart.c > @@ -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; > } >
On Sat, 12 May 2018 09:51:02 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Thu, 12 Apr 2018 07:24:52 +0200 > Rafał Miłecki <zajec5@gmail.com> wrote: > > > From: Rafał Miłecki <rafal@milecki.pl> > > > > 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 > > > > > 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. > > I didn't check the TRX parsing logic, so I'm assuming you know what you > do here and you've tested the modifications ;-). > > > > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > --- > > drivers/mtd/bcm47xxpart.c | 22 ++++++++++++++++++---- > > 1 file changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c > > index fe2581d9d882..1f0239848ebe 100644 > > --- a/drivers/mtd/bcm47xxpart.c > > +++ b/drivers/mtd/bcm47xxpart.c > > @@ -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; > > } > > > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c index fe2581d9d882..1f0239848ebe 100644 --- a/drivers/mtd/bcm47xxpart.c +++ b/drivers/mtd/bcm47xxpart.c @@ -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; }