[PATCH] mtd: bcm47xxpart: improve handling TRX partition size

[PATCH] mtd: bcm47xxpart: improve handling TRX partition size

[Rafał Miłecki]

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.

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.

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;
 		}
 
-- 
2.12.0

Re: [PATCH] mtd: bcm47xxpart: improve handling TRX partition size

[Boris Brezillon]

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;
>  		}
>  

Re: [PATCH] mtd: bcm47xxpart: improve handling TRX partition size

[Boris Brezillon]

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/