mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <uwe@kleine-koenig.org>
To: Trent Piepho <tpiepho@kymetacorp.com>
Cc: "barebox@lists.infradead.org" <barebox@lists.infradead.org>
Subject: Re: [PATCH v2] kwboot: do a filetype check before sending the image
Date: Thu, 21 Jan 2016 11:13:05 +0100	[thread overview]
Message-ID: <56A0AF31.3040904@kleine-koenig.org> (raw)
In-Reply-To: <1453318040.4474.287.camel@rtred1test09.kymeta.local>


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

  reply	other threads:[~2016-01-21 10:13 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-20  8:15 Uwe Kleine-König
2016-01-20 11:54 ` Sebastian Hesselbarth
2016-01-20 19:27 ` Trent Piepho
2016-01-21 10:13   ` Uwe Kleine-König [this message]
2016-01-21 20:12     ` Trent Piepho

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56A0AF31.3040904@kleine-koenig.org \
    --to=uwe@kleine-koenig.org \
    --cc=barebox@lists.infradead.org \
    --cc=tpiepho@kymetacorp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox