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

["Uwe Kleine-König" <uwe@kleine-koenig.org>]

[-- 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