diff mbox

[U-Boot,V2,02/21] imximage: check dcd_len as entries added

Message ID 1348281558-19520-3-git-send-email-troy.kisky@boundarydevices.com
State Changes Requested
Headers show

Commit Message

Troy Kisky Sept. 22, 2012, 2:38 a.m. UTC 
Before the len was checked after the entire file
was processed, so it could have already overflowed.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 tools/imximage.c |   26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

Comments

Stefano Babic Sept. 23, 2012, 11:05 a.m. UTC | #1 
On 22/09/2012 04:38, Troy Kisky wrote:
> Before the len was checked after the entire file
> was processed, so it could have already overflowed.
> 

Hi Troy,

> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
>  tools/imximage.c |   26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 25d3b74..0bfbec3 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -71,6 +71,7 @@ static set_dcd_val_t set_dcd_val;
>  static set_dcd_rst_t set_dcd_rst;
>  static set_imx_hdr_t set_imx_hdr;
>  static set_imx_size_t set_imx_size;
> +static uint32_t max_dcd_entries;
>  static uint32_t g_flash_offset;
>  
>  static struct image_type_params imximage_params;
> @@ -173,13 +174,6 @@ static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len,
>  {
>  	dcd_v1_t *dcd_v1 = &imxhdr->header.hdr_v1.dcd_table;
>  
> -	if (dcd_len > MAX_HW_CFG_SIZE_V1) {
> -		fprintf(stderr, "Error: %s[%d] -"
> -			"DCD table exceeds maximum size(%d)\n",
> -			name, lineno, MAX_HW_CFG_SIZE_V1);
> -		exit(EXIT_FAILURE);
> -	}
> -
>  	dcd_v1->preamble.barker = DCD_BARKER;
>  	dcd_v1->preamble.length = dcd_len * sizeof(dcd_type_addr_data_t);
>  }
> @@ -193,13 +187,6 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>  {
>  	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
>  
> -	if (dcd_len > MAX_HW_CFG_SIZE_V2) {
> -		fprintf(stderr, "Error: %s[%d] -"
> -			"DCD table exceeds maximum size(%d)\n",
> -			name, lineno, MAX_HW_CFG_SIZE_V2);
> -		exit(EXIT_FAILURE);
> -	}
> -
>  	dcd_v2->header.tag = DCD_HEADER_TAG;
>  	dcd_v2->header.length = cpu_to_be16(
>  			dcd_len * sizeof(dcd_addr_data_t) + 8);
> @@ -293,12 +280,14 @@ static void set_hdr_func(struct imx_header *imxhdr)
>  		set_dcd_rst = set_dcd_rst_v1;
>  		set_imx_hdr = set_imx_hdr_v1;
>  		set_imx_size = set_imx_size_v1;
> +		max_dcd_entries = MAX_HW_CFG_SIZE_V1;
>  		break;
>  	case IMXIMAGE_V2:
>  		set_dcd_val = set_dcd_val_v2;
>  		set_dcd_rst = set_dcd_rst_v2;
>  		set_imx_hdr = set_imx_hdr_v2;
>  		set_imx_size = set_imx_size_v2;
> +		max_dcd_entries = MAX_HW_CFG_SIZE_V2;
>  		break;
>  	default:
>  		err_imximage_version(imximage_version);
> @@ -425,8 +414,15 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
>  		value = get_cfg_value(token, name, lineno);
>  		(*set_dcd_val)(imxhdr, name, lineno, fld, value, *dcd_len);
>  
> -		if (fld == CFG_REG_VALUE)
> +		if (fld == CFG_REG_VALUE) {
>  			(*dcd_len)++;
> +			if (*dcd_len > max_dcd_entries) {
> +				fprintf(stderr, "Error: %s[%d] -"
> +					"DCD table exceeds maximum size(%d)\n",
> +					name, lineno, max_dcd_entries);
> +				exit(EXIT_FAILURE);
> +			}
> +		}
>  		break;
>  	default:
>  		break;
> 

This patch seems to me unrelated to the rest, and fixes the case when
too much DCD entries are put into the imximage.cfg file. What about to
rebase it on the current code and post it as separate patch ? I think
this can be merged directly, also in the current realease.

Best regards,
Stefano Babic
Troy Kisky Sept. 24, 2012, 8:54 p.m. UTC | #2 
On 9/23/2012 4:05 AM, Stefano Babic wrote:
> On 22/09/2012 04:38, Troy Kisky wrote:
>> Before the len was checked after the entire file
>> was processed, so it could have already overflowed.
>>
> Hi Troy,
>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>> ---
>>   tools/imximage.c |   26 +++++++++++---------------
>>   1 file changed, 11 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/imximage.c b/tools/imximage.c
>> index 25d3b74..0bfbec3 100644
>> --- a/tools/imximage.c
>> +++ b/tools/imximage.c
>> @@ -71,6 +71,7 @@ static set_dcd_val_t set_dcd_val;
>>   static set_dcd_rst_t set_dcd_rst;
>>   static set_imx_hdr_t set_imx_hdr;
>>   static set_imx_size_t set_imx_size;
>> +static uint32_t max_dcd_entries;
>>   static uint32_t g_flash_offset;
>>   
>>   static struct image_type_params imximage_params;
>> @@ -173,13 +174,6 @@ static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len,
>>   {
>>   	dcd_v1_t *dcd_v1 = &imxhdr->header.hdr_v1.dcd_table;
>>   
>> -	if (dcd_len > MAX_HW_CFG_SIZE_V1) {
>> -		fprintf(stderr, "Error: %s[%d] -"
>> -			"DCD table exceeds maximum size(%d)\n",
>> -			name, lineno, MAX_HW_CFG_SIZE_V1);
>> -		exit(EXIT_FAILURE);
>> -	}
>> -
>>   	dcd_v1->preamble.barker = DCD_BARKER;
>>   	dcd_v1->preamble.length = dcd_len * sizeof(dcd_type_addr_data_t);
>>   }
>> @@ -193,13 +187,6 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>>   {
>>   	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
>>   
>> -	if (dcd_len > MAX_HW_CFG_SIZE_V2) {
>> -		fprintf(stderr, "Error: %s[%d] -"
>> -			"DCD table exceeds maximum size(%d)\n",
>> -			name, lineno, MAX_HW_CFG_SIZE_V2);
>> -		exit(EXIT_FAILURE);
>> -	}
>> -
>>   	dcd_v2->header.tag = DCD_HEADER_TAG;
>>   	dcd_v2->header.length = cpu_to_be16(
>>   			dcd_len * sizeof(dcd_addr_data_t) + 8);
>> @@ -293,12 +280,14 @@ static void set_hdr_func(struct imx_header *imxhdr)
>>   		set_dcd_rst = set_dcd_rst_v1;
>>   		set_imx_hdr = set_imx_hdr_v1;
>>   		set_imx_size = set_imx_size_v1;
>> +		max_dcd_entries = MAX_HW_CFG_SIZE_V1;
>>   		break;
>>   	case IMXIMAGE_V2:
>>   		set_dcd_val = set_dcd_val_v2;
>>   		set_dcd_rst = set_dcd_rst_v2;
>>   		set_imx_hdr = set_imx_hdr_v2;
>>   		set_imx_size = set_imx_size_v2;
>> +		max_dcd_entries = MAX_HW_CFG_SIZE_V2;
>>   		break;
>>   	default:
>>   		err_imximage_version(imximage_version);
>> @@ -425,8 +414,15 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
>>   		value = get_cfg_value(token, name, lineno);
>>   		(*set_dcd_val)(imxhdr, name, lineno, fld, value, *dcd_len);
>>   
>> -		if (fld == CFG_REG_VALUE)
>> +		if (fld == CFG_REG_VALUE) {
>>   			(*dcd_len)++;
>> +			if (*dcd_len > max_dcd_entries) {
>> +				fprintf(stderr, "Error: %s[%d] -"
>> +					"DCD table exceeds maximum size(%d)\n",
>> +					name, lineno, max_dcd_entries);
>> +				exit(EXIT_FAILURE);
>> +			}
>> +		}
>>   		break;
>>   	default:
>>   		break;
>>
> This patch seems to me unrelated to the rest, and fixes the case when
> too much DCD entries are put into the imximage.cfg file. What about to
> rebase it on the current code and post it as separate patch ? I think
> this can be merged directly, also in the current realease.
>
> Best regards,
> Stefano Babic
>

It is a fix, but for a bug that has never happened. So I think it is 
very low priority.
But I can reorder the patches so that this is the 1st in the series, in case
the other patches are never accepted.
I don't think it belongs in the current release.

Troy
Stefano Babic Sept. 25, 2012, 11:12 a.m. UTC | #3 
On 24/09/2012 22:54, Troy Kisky wrote:

> It is a fix, but for a bug that has never happened.

Right, but anyway it is a fix ;-)

> So I think it is
> very low priority.
> But I can reorder the patches so that this is the 1st in the series, in
> case
> the other patches are never accepted.

The reason is that simply and easy patches could be merged soon.

> I don't think it belongs in the current release.

Right, this should go in -next

Best regards,
Stefano
diff mbox

Patch

diff --git a/tools/imximage.c b/tools/imximage.c
index 25d3b74..0bfbec3 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -71,6 +71,7 @@  static set_dcd_val_t set_dcd_val;
 static set_dcd_rst_t set_dcd_rst;
 static set_imx_hdr_t set_imx_hdr;
 static set_imx_size_t set_imx_size;
+static uint32_t max_dcd_entries;
 static uint32_t g_flash_offset;
 
 static struct image_type_params imximage_params;
@@ -173,13 +174,6 @@  static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len,
 {
 	dcd_v1_t *dcd_v1 = &imxhdr->header.hdr_v1.dcd_table;
 
-	if (dcd_len > MAX_HW_CFG_SIZE_V1) {
-		fprintf(stderr, "Error: %s[%d] -"
-			"DCD table exceeds maximum size(%d)\n",
-			name, lineno, MAX_HW_CFG_SIZE_V1);
-		exit(EXIT_FAILURE);
-	}
-
 	dcd_v1->preamble.barker = DCD_BARKER;
 	dcd_v1->preamble.length = dcd_len * sizeof(dcd_type_addr_data_t);
 }
@@ -193,13 +187,6 @@  static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
 {
 	dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table;
 
-	if (dcd_len > MAX_HW_CFG_SIZE_V2) {
-		fprintf(stderr, "Error: %s[%d] -"
-			"DCD table exceeds maximum size(%d)\n",
-			name, lineno, MAX_HW_CFG_SIZE_V2);
-		exit(EXIT_FAILURE);
-	}
-
 	dcd_v2->header.tag = DCD_HEADER_TAG;
 	dcd_v2->header.length = cpu_to_be16(
 			dcd_len * sizeof(dcd_addr_data_t) + 8);
@@ -293,12 +280,14 @@  static void set_hdr_func(struct imx_header *imxhdr)
 		set_dcd_rst = set_dcd_rst_v1;
 		set_imx_hdr = set_imx_hdr_v1;
 		set_imx_size = set_imx_size_v1;
+		max_dcd_entries = MAX_HW_CFG_SIZE_V1;
 		break;
 	case IMXIMAGE_V2:
 		set_dcd_val = set_dcd_val_v2;
 		set_dcd_rst = set_dcd_rst_v2;
 		set_imx_hdr = set_imx_hdr_v2;
 		set_imx_size = set_imx_size_v2;
+		max_dcd_entries = MAX_HW_CFG_SIZE_V2;
 		break;
 	default:
 		err_imximage_version(imximage_version);
@@ -425,8 +414,15 @@  static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd,
 		value = get_cfg_value(token, name, lineno);
 		(*set_dcd_val)(imxhdr, name, lineno, fld, value, *dcd_len);
 
-		if (fld == CFG_REG_VALUE)
+		if (fld == CFG_REG_VALUE) {
 			(*dcd_len)++;
+			if (*dcd_len > max_dcd_entries) {
+				fprintf(stderr, "Error: %s[%d] -"
+					"DCD table exceeds maximum size(%d)\n",
+					name, lineno, max_dcd_entries);
+				exit(EXIT_FAILURE);
+			}
+		}
 		break;
 	default:
 		break;