[PATCH v2] kwboot: do a filetype check before sending the image

[PATCH v2] kwboot: do a filetype check before sending the image

[Uwe Kleine-König]

The images that can be sent to a Marvell CPU have a fixed format. Do
some sanity checks before actually sending an image for easier diagnosis
of broken files.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
Changes since (implicit) v1, sent with
Message-Id: 1453276010-4669-1-git-send-email-uwe@kleine-koenig.org:

 - whitespace fix
 - error out if a problem is detected
 - add a commit log

 scripts/kwboot.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/scripts/kwboot.c b/scripts/kwboot.c
index 46328d8ed006..06e58f6a7e3b 100644
--- a/scripts/kwboot.c
+++ b/scripts/kwboot.c
@@ -546,6 +546,59 @@ out:
 	return rc;
 }
 
+static int
+kwboot_check_image(unsigned char *img, size_t size)
+{
+	size_t i;
+	size_t header_size, image_size;
+	unsigned char csum = 0;
+
+	switch (img[0x0]) {
+		case 0x5a: /* SPI/NOR */
+		case 0x69: /* UART0 */
+		case 0x78: /* SATA */
+		case 0x8b: /* NAND */
+		case 0x9c: /* PCIe */
+			break;
+		default:
+			printf("Unknown boot source: 0x%hhx\n", img[0x0]);
+			goto err;
+	}
+
+	if (img[0x8] != 1) {
+		printf("Unknown version: 0x%hhx\n", img[0x8]);
+		goto err;
+	}
+
+	image_size = img[0x4] | (img[0x5] << 8) |
+		(img[0x6] << 16) | (img[0x7] << 24);
+
+	header_size = (img[0x9] << 16) | img[0xa] | (img[0xb] << 8);
+
+	if (header_size + image_size != size) {
+		printf("Size mismatch (%zu + %zu != %zu)\n",
+		       header_size, image_size, size);
+		goto err;
+	} else {
+		for (i = 0; i < header_size; ++i)
+			csum += img[i];
+
+		csum -= img[0x1f];
+
+		if (csum != img[0x1f]) {
+			printf("Checksum mismatch: header: 0x%02hhx, calculated: 0x%02hhx\n",
+			       img[0x1f], csum);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	errno = EINVAL;
+	return 1;
+}
+
 static void *
 kwboot_mmap_image(const char *path, size_t *size, int prot)
 {
@@ -574,6 +627,8 @@ kwboot_mmap_image(const char *path, size_t *size, int prot)
 
 	rc = 0;
 	*size = st.st_size;
+
+	rc = kwboot_check_image(img, *size);
 out:
 	if (rc && img) {
 		munmap(img, st.st_size);
-- 
2.7.0.rc3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] kwboot: do a filetype check before sending the image

[Sebastian Hesselbarth]

On 01/20/2016 09:15 AM, Uwe Kleine-König wrote:
> The images that can be sent to a Marvell CPU have a fixed format. Do
> some sanity checks before actually sending an image for easier diagnosis
> of broken files.
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Changes since (implicit) v1, sent with
> Message-Id: 1453276010-4669-1-git-send-email-uwe@kleine-koenig.org:
>
>   - whitespace fix
>   - error out if a problem is detected
>   - add a commit log
>
>   scripts/kwboot.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>
> diff --git a/scripts/kwboot.c b/scripts/kwboot.c
> index 46328d8ed006..06e58f6a7e3b 100644
> --- a/scripts/kwboot.c
> +++ b/scripts/kwboot.c
> @@ -546,6 +546,59 @@ out:
>   	return rc;
>   }
>
> +static int
> +kwboot_check_image(unsigned char *img, size_t size)
> +{
> +	size_t i;
> +	size_t header_size, image_size;
> +	unsigned char csum = 0;
> +
> +	switch (img[0x0]) {
> +		case 0x5a: /* SPI/NOR */
> +		case 0x69: /* UART0 */
> +		case 0x78: /* SATA */
> +		case 0x8b: /* NAND */
> +		case 0x9c: /* PCIe */
> +			break;
> +		default:
> +			printf("Unknown boot source: 0x%hhx\n", img[0x0]);
> +			goto err;
> +	}
> +
> +	if (img[0x8] != 1) {
> +		printf("Unknown version: 0x%hhx\n", img[0x8]);
> +		goto err;
> +	}
> +
> +	image_size = img[0x4] | (img[0x5] << 8) |
> +		(img[0x6] << 16) | (img[0x7] << 24);
> +
> +	header_size = (img[0x9] << 16) | img[0xa] | (img[0xb] << 8);
> +
> +	if (header_size + image_size != size) {
> +		printf("Size mismatch (%zu + %zu != %zu)\n",
> +		       header_size, image_size, size);
> +		goto err;
> +	} else {
> +		for (i = 0; i < header_size; ++i)
> +			csum += img[i];
> +
> +		csum -= img[0x1f];
> +
> +		if (csum != img[0x1f]) {
> +			printf("Checksum mismatch: header: 0x%02hhx, calculated: 0x%02hhx\n",
> +			       img[0x1f], csum);
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	errno = EINVAL;
> +	return 1;
> +}
> +
>   static void *
>   kwboot_mmap_image(const char *path, size_t *size, int prot)
>   {
> @@ -574,6 +627,8 @@ kwboot_mmap_image(const char *path, size_t *size, int prot)
>
>   	rc = 0;
>   	*size = st.st_size;
> +
> +	rc = kwboot_check_image(img, *size);

Uwe,

while I like the check, a real "elite" hacking tool should always give
you the option to overrule any checks.

Mind to also add a "force" option to send the image regardless of the
outcome of kwboot_check_image?

Sebastian

>   out:
>   	if (rc && img) {
>   		munmap(img, st.st_size);
>

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] kwboot: do a filetype check before sending the image

[Trent Piepho]

On Wed, 2016-01-20 at 09:15 +0100, Uwe Kleine-König wrote:
> The images that can be sent to a Marvell CPU have a fixed format. Do
> some sanity checks before actually sending an image for easier diagnosis
> of broken files.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Changes since (implicit) v1, sent with
> Message-Id: 1453276010-4669-1-git-send-email-uwe@kleine-koenig.org:
> 
>  - whitespace fix
>  - error out if a problem is detected
>  - add a commit log
> 
>  scripts/kwboot.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/scripts/kwboot.c b/scripts/kwboot.c
> index 46328d8ed006..06e58f6a7e3b 100644
> --- a/scripts/kwboot.c
> +++ b/scripts/kwboot.c
> @@ -546,6 +546,59 @@ out:
>  	return rc;
>  }
>  
> +static int
> +kwboot_check_image(unsigned char *img, size_t size)

Make it const unsigned char?

> +{
> +	size_t i;
> +	size_t header_size, image_size;
> +	unsigned char csum = 0;
> +
> +	switch (img[0x0]) {
> +		case 0x5a: /* SPI/NOR */
> +		case 0x69: /* UART0 */
> +		case 0x78: /* SATA */
> +		case 0x8b: /* NAND */
> +		case 0x9c: /* PCIe */
> +			break;
> +		default:
> +			printf("Unknown boot source: 0x%hhx\n", img[0x0]);
> +			goto err;
> +	}
> +
> +	if (img[0x8] != 1) {
> +		printf("Unknown version: 0x%hhx\n", img[0x8]);
> +		goto err;
> +	}

If you're verifying the image, maybe also check that size > 8 before
reading img[0x8]?  Otherwise you could crash instead of rejecting a too
small image.

And having compared the version tag to 1, couldn't you then cast img
into a struct main_hdr_v1 *?  That would avoid all the hard coded magic
offsets in the rest of the code.  Unless img isn't aligned?

> +
> +	image_size = img[0x4] | (img[0x5] << 8) |
> +		(img[0x6] << 16) | (img[0x7] << 24);

struct main_hdr_v1 *hdr = (const struct main_hdr
image_size = le32_to_cpu(hdr->blocksize);
or if unaligned:
image_size = get_unaligned_le32(&img[0x4]);

> +
> +	header_size = (img[0x9] << 16) | img[0xa] | (img[0xb] << 8);

header_size = hdr->headersz_msb << 16 | le16_to_cpu(hdr->headersz_lsb);

> +
> +	if (header_size + image_size != size) {
> +		printf("Size mismatch (%zu + %zu != %zu)\n",
> +		       header_size, image_size, size);
> +		goto err;
> +	} else {

Don't really need the else block here since the failure above exits,
just like the two failure checks before this one.

> +		for (i = 0; i < header_size; ++i)
> +			csum += img[i];

csum = image_checksum8(img, header_size)

> +
> +		csum -= img[0x1f];
> +
> +		if (csum != img[0x1f]) {
> +			printf("Checksum mismatch: header: 0x%02hhx, calculated: 0x%02hhx\n",
> +			       img[0x1f], csum);
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	errno = EINVAL;
> +	return 1;
> +}
> +
>  static void *
>  kwboot_mmap_image(const char *path, size_t *size, int prot)
>  {
> @@ -574,6 +627,8 @@ kwboot_mmap_image(const char *path, size_t *size, int prot)
>  
>  	rc = 0;
>  	*size = st.st_size;
> +
> +	rc = kwboot_check_image(img, *size);
>  out:
>  	if (rc && img) {
>  		munmap(img, st.st_size);

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] kwboot: do a filetype check before sending the image

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3362 bytes --]

Hello Trent,

On 01/20/2016 08:27 PM, Trent Piepho wrote:
> On Wed, 2016-01-20 at 09:15 +0100, Uwe Kleine-König wrote:
>> The images that can be sent to a Marvell CPU have a fixed format. Do
>> some sanity checks before actually sending an image for easier diagnosis
>> of broken files.
>>
>> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>> ---
>> Changes since (implicit) v1, sent with
>> Message-Id: 1453276010-4669-1-git-send-email-uwe@kleine-koenig.org:
>>
>>  - whitespace fix
>>  - error out if a problem is detected
>>  - add a commit log
>>
>>  scripts/kwboot.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>
>> diff --git a/scripts/kwboot.c b/scripts/kwboot.c
>> index 46328d8ed006..06e58f6a7e3b 100644
>> --- a/scripts/kwboot.c
>> +++ b/scripts/kwboot.c
>> @@ -546,6 +546,59 @@ out:
>>  	return rc;
>>  }
>>  
>> +static int
>> +kwboot_check_image(unsigned char *img, size_t size)
> 
> Make it const unsigned char?

sure

>> +{
>> +	size_t i;
>> +	size_t header_size, image_size;
>> +	unsigned char csum = 0;
>> +
>> +	switch (img[0x0]) {
>> +		case 0x5a: /* SPI/NOR */
>> +		case 0x69: /* UART0 */
>> +		case 0x78: /* SATA */
>> +		case 0x8b: /* NAND */
>> +		case 0x9c: /* PCIe */
>> +			break;
>> +		default:
>> +			printf("Unknown boot source: 0x%hhx\n", img[0x0]);
>> +			goto err;
>> +	}
>> +
>> +	if (img[0x8] != 1) {
>> +		printf("Unknown version: 0x%hhx\n", img[0x8]);
>> +		goto err;
>> +	}
> 
> If you're verifying the image, maybe also check that size > 8 before
> reading img[0x8]?  Otherwise you could crash instead of rejecting a too
> small image.

good catch

> And having compared the version tag to 1, couldn't you then cast img
> into a struct main_hdr_v1 *?  That would avoid all the hard coded magic
> offsets in the rest of the code.  Unless img isn't aligned?

... or the build machine is big endian. (Note in this case kwbimage is
broken, too. And given that catching such an error independently is IMHO
a feature.) I'd prefer to keep it as is at least for now for a few
reasons (endianess, alignment, bigger reshuffling of code because said
struct is defined in a different .c file)

>> +
>> +	image_size = img[0x4] | (img[0x5] << 8) |
>> +		(img[0x6] << 16) | (img[0x7] << 24);
> 
> struct main_hdr_v1 *hdr = (const struct main_hdr
> image_size = le32_to_cpu(hdr->blocksize);
> or if unaligned:
> image_size = get_unaligned_le32(&img[0x4]);

Of the three offered versions I'd prefer mine ... It's simple and easy
to verify when comparing to the reference manual.

>> +
>> +	header_size = (img[0x9] << 16) | img[0xa] | (img[0xb] << 8);
> 
> header_size = hdr->headersz_msb << 16 | le16_to_cpu(hdr->headersz_lsb);
> 
>> +
>> +	if (header_size + image_size != size) {
>> +		printf("Size mismatch (%zu + %zu != %zu)\n",
>> +		       header_size, image_size, size);
>> +		goto err;
>> +	} else {
> 
> Don't really need the else block here since the failure above exits,
> just like the two failure checks before this one.

right, this is a relict of v1 where I didn't jump in the first block.

>> +		for (i = 0; i < header_size; ++i)
>> +			csum += img[i];
> 
> csum = image_checksum8(img, header_size)

This is again in kwbimage.c

Best regards
Uwe


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] kwboot: do a filetype check before sending the image

[Trent Piepho]

On Thu, 2016-01-21 at 11:13 +0100, Uwe Kleine-König wrote:

> > And having compared the version tag to 1, couldn't you then cast img
> > into a struct main_hdr_v1 *?  That would avoid all the hard coded magic
> > offsets in the rest of the code.  Unless img isn't aligned?
> 
> ... or the build machine is big endian. (Note in this case kwbimage is
> broken, too. And given that catching such an error independently is IMHO
> a feature.) I'd prefer to keep it as is at least for now for a few
> reasons (endianess, alignment, bigger reshuffling of code because said
> struct is defined in a different .c file)

Endianess shouldn't make a difference when it comes to using the struct.
Seems like it would be nice to have a common definition of the structs
between the code that makes the image and the code the reads it.


> >> +
> >> +	image_size = img[0x4] | (img[0x5] << 8) |
> >> +		(img[0x6] << 16) | (img[0x7] << 24);
> > 
> > struct main_hdr_v1 *hdr = (const struct main_hdr
> > image_size = le32_to_cpu(hdr->blocksize);
> > or if unaligned:
> > image_size = get_unaligned_le32(&img[0x4]);
> 
> Of the three offered versions I'd prefer mine ... It's simple and easy
> to verify when comparing to the reference manual.
> 
> >> +
> >> +	header_size = (img[0x9] << 16) | img[0xa] | (img[0xb] << 8);
> > 
> > header_size = hdr->headersz_msb << 16 | le16_to_cpu(hdr->headersz_lsb);
> > 
> >> +
> >> +	if (header_size + image_size != size) {
> >> +		printf("Size mismatch (%zu + %zu != %zu)\n",
> >> +		       header_size, image_size, size);
> >> +		goto err;
> >> +	} else {
> > 
> > Don't really need the else block here since the failure above exits,
> > just like the two failure checks before this one.
> 
> right, this is a relict of v1 where I didn't jump in the first block.
> 
> >> +		for (i = 0; i < header_size; ++i)
> >> +			csum += img[i];
> > 
> > csum = image_checksum8(img, header_size)
> 
> This is again in kwbimage.c
> 
> Best regards
> Uwe
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox