[U-Boot] [PATCH 1/2] gadget: f_thor: fix filename overflow

[U-Boot] [PATCH 1/2] gadget: f_thor: fix filename overflow

[Seung-Woo Kim]

The thor sender can send filename without null character and it is
used without consideration of overflow. Actually, character array
for filename is assigned with DEFINE_CACHE_ALIGN_BUFFER() and it
is bigger than size of memcpy, so there was no real overflow.
Fix filename overflow for code level integrity.

Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
 drivers/usb/gadget/f_thor.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
index f874509..6d38cb6 100644
--- a/drivers/usb/gadget/f_thor.c
+++ b/drivers/usb/gadget/f_thor.c
@@ -47,7 +47,7 @@ DEFINE_CACHE_ALIGN_BUFFER(unsigned char, thor_rx_data_buf,
 /* ********************************************************** */
 /*         THOR protocol - transmission handling	      */
 /* ********************************************************** */
-DEFINE_CACHE_ALIGN_BUFFER(char, f_name, F_NAME_BUF_SIZE);
+DEFINE_CACHE_ALIGN_BUFFER(char, f_name, F_NAME_BUF_SIZE + 1);
 static unsigned long long int thor_file_size;
 static int alt_setting_num;
 
@@ -276,6 +276,7 @@ static long long int process_rqt_download(const struct rqt_box *rqt)
 
 		thor_file_size = rqt->int_data[1];
 		memcpy(f_name, rqt->str_data[0], F_NAME_BUF_SIZE);
+		f_name[F_NAME_BUF_SIZE] = '\0';
 
 		debug("INFO: name(%s, %d), size(%llu), type(%d)\n",
 		      f_name, 0, thor_file_size, file_type);
-- 
1.7.9.5

[U-Boot] [PATCH 2/2] gadget: f_thor: update to support more than 4GB file as thor 5.0

[Seung-Woo Kim]

During file download, it only uses 32bit variable for file size and
it limits maximum file size less than 4GB. Update to support more
than 4GB file with using two 32bit variables for file size as thor
protocol 5.0.

Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
 drivers/usb/gadget/f_thor.c |   10 +++++++---
 drivers/usb/gadget/f_thor.h |    2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
index 6d38cb6..c8eda05 100644
--- a/drivers/usb/gadget/f_thor.c
+++ b/drivers/usb/gadget/f_thor.c
@@ -262,8 +262,10 @@ static long long int process_rqt_download(const struct rqt_box *rqt)
 
 	switch (rqt->rqt_data) {
 	case RQT_DL_INIT:
-		thor_file_size = rqt->int_data[0];
-		debug("INIT: total %d bytes\n", rqt->int_data[0]);
+		thor_file_size = (unsigned long long int)rqt->int_data[0] +
+				 (((unsigned long long int)rqt->int_data[1])
+				  << 32);
+		debug("INIT: total %llu bytes\n", thor_file_size);
 		break;
 	case RQT_DL_FILE_INFO:
 		file_type = rqt->int_data[0];
@@ -274,7 +276,9 @@ static long long int process_rqt_download(const struct rqt_box *rqt)
 			break;
 		}
 
-		thor_file_size = rqt->int_data[1];
+		thor_file_size = (unsigned long long int)rqt->int_data[1] +
+				 (((unsigned long long int)rqt->int_data[2])
+				  << 32);
 		memcpy(f_name, rqt->str_data[0], F_NAME_BUF_SIZE);
 		f_name[F_NAME_BUF_SIZE] = '\0';
 
diff --git a/drivers/usb/gadget/f_thor.h b/drivers/usb/gadget/f_thor.h
index 47abc8a..8ba3fa2 100644
--- a/drivers/usb/gadget/f_thor.h
+++ b/drivers/usb/gadget/f_thor.h
@@ -34,7 +34,7 @@ struct usb_cdc_attribute_vendor_descriptor {
 	__u8 DAUValue;
 } __packed;
 
-#define VER_PROTOCOL_MAJOR	4
+#define VER_PROTOCOL_MAJOR	5
 #define VER_PROTOCOL_MINOR	0
 
 enum rqt {
-- 
1.7.9.5

[U-Boot] [PATCH 2/2] gadget: f_thor: update to support more than 4GB file as thor 5.0

[Lukasz Majewski]

Hi Seung-Woo,

> During file download, it only uses 32bit variable for file size and
> it limits maximum file size less than 4GB. Update to support more
> than 4GB file with using two 32bit variables for file size as thor
> protocol 5.0.

I assume that it was also tested that this patch will not break devices
already using protocol version 4 (like some hobbysts trats2 users, or
odroid XU3)?

To be more specific - is the init_data[1] zeroed in the earlier version
(version 4) of the THOR protocol?

> 
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> ---
>  drivers/usb/gadget/f_thor.c |   10 +++++++---
>  drivers/usb/gadget/f_thor.h |    2 +-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
> index 6d38cb6..c8eda05 100644
> --- a/drivers/usb/gadget/f_thor.c
> +++ b/drivers/usb/gadget/f_thor.c
> @@ -262,8 +262,10 @@ static long long int process_rqt_download(const
> struct rqt_box *rqt) 
>  	switch (rqt->rqt_data) {
>  	case RQT_DL_INIT:
> -		thor_file_size = rqt->int_data[0];
> -		debug("INIT: total %d bytes\n", rqt->int_data[0]);
> +		thor_file_size = (unsigned long long
> int)rqt->int_data[0] +
> +				 (((unsigned long long
> int)rqt->int_data[1])
> +				  << 32);
> +		debug("INIT: total %llu bytes\n", thor_file_size);
>  		break;
>  	case RQT_DL_FILE_INFO:
>  		file_type = rqt->int_data[0];
> @@ -274,7 +276,9 @@ static long long int process_rqt_download(const
> struct rqt_box *rqt) break;
>  		}
>  
> -		thor_file_size = rqt->int_data[1];
> +		thor_file_size = (unsigned long long
> int)rqt->int_data[1] +
> +				 (((unsigned long long
> int)rqt->int_data[2])
> +				  << 32);
>  		memcpy(f_name, rqt->str_data[0], F_NAME_BUF_SIZE);
>  		f_name[F_NAME_BUF_SIZE] = '\0';
>  
> diff --git a/drivers/usb/gadget/f_thor.h b/drivers/usb/gadget/f_thor.h
> index 47abc8a..8ba3fa2 100644
> --- a/drivers/usb/gadget/f_thor.h
> +++ b/drivers/usb/gadget/f_thor.h
> @@ -34,7 +34,7 @@ struct usb_cdc_attribute_vendor_descriptor {
>  	__u8 DAUValue;
>  } __packed;
>  
> -#define VER_PROTOCOL_MAJOR	4
> +#define VER_PROTOCOL_MAJOR	5
>  #define VER_PROTOCOL_MINOR	0
>  
>  enum rqt {

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180510/3d50e8e3/attachment.sig>

[U-Boot] [PATCH 2/2] gadget: f_thor: update to support more than 4GB file as thor 5.0

[Seung-Woo Kim]

Hello Lukasz,

On 2018년 05월 10일 19:58, Lukasz Majewski wrote:
> Hi Seung-Woo,
> 
>> During file download, it only uses 32bit variable for file size and
>> it limits maximum file size less than 4GB. Update to support more
>> than 4GB file with using two 32bit variables for file size as thor
>> protocol 5.0.
> 
> I assume that it was also tested that this patch will not break devices
> already using protocol version 4 (like some hobbysts trats2 users, or
> odroid XU3)?

Yes, of course. I have checked all those devices are using THOR protocol
version 4.0 and tested with the devices.

> 
> To be more specific - is the init_data[1] zeroed in the earlier version
> (version 4) of the THOR protocol?

From the thor tool like Tizen lthor[1], it clears all request.

And THOR protocol 5.0 support has also done from Tizen lthor  and it is
currently under review. I have tested THOR protocol 5.0 device with THOR
protocol 4.0 lthor and reserve way also. From lthor with THOR protocol
5.0, I just added to check protocol version of target and if the target
protocol version is less than 5.0 than I made lthor refuse 4GB or larger
file.

[1]: https://git.tizen.org/cgit/tools/lthor

Best Regards,
- Seung-Woo Kim

> 
>>
>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
>> ---
>>  drivers/usb/gadget/f_thor.c |   10 +++++++---
>>  drivers/usb/gadget/f_thor.h |    2 +-
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
>> index 6d38cb6..c8eda05 100644
>> --- a/drivers/usb/gadget/f_thor.c
>> +++ b/drivers/usb/gadget/f_thor.c
>> @@ -262,8 +262,10 @@ static long long int process_rqt_download(const
>> struct rqt_box *rqt) 
>>  	switch (rqt->rqt_data) {
>>  	case RQT_DL_INIT:
>> -		thor_file_size = rqt->int_data[0];
>> -		debug("INIT: total %d bytes\n", rqt->int_data[0]);
>> +		thor_file_size = (unsigned long long
>> int)rqt->int_data[0] +
>> +				 (((unsigned long long
>> int)rqt->int_data[1])
>> +				  << 32);
>> +		debug("INIT: total %llu bytes\n", thor_file_size);
>>  		break;
>>  	case RQT_DL_FILE_INFO:
>>  		file_type = rqt->int_data[0];
>> @@ -274,7 +276,9 @@ static long long int process_rqt_download(const
>> struct rqt_box *rqt) break;
>>  		}
>>  
>> -		thor_file_size = rqt->int_data[1];
>> +		thor_file_size = (unsigned long long
>> int)rqt->int_data[1] +
>> +				 (((unsigned long long
>> int)rqt->int_data[2])
>> +				  << 32);
>>  		memcpy(f_name, rqt->str_data[0], F_NAME_BUF_SIZE);
>>  		f_name[F_NAME_BUF_SIZE] = '\0';
>>  
>> diff --git a/drivers/usb/gadget/f_thor.h b/drivers/usb/gadget/f_thor.h
>> index 47abc8a..8ba3fa2 100644
>> --- a/drivers/usb/gadget/f_thor.h
>> +++ b/drivers/usb/gadget/f_thor.h
>> @@ -34,7 +34,7 @@ struct usb_cdc_attribute_vendor_descriptor {
>>  	__u8 DAUValue;
>>  } __packed;
>>  
>> -#define VER_PROTOCOL_MAJOR	4
>> +#define VER_PROTOCOL_MAJOR	5
>>  #define VER_PROTOCOL_MINOR	0
>>  
>>  enum rqt {
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> 

-- 
Seung-Woo Kim
Samsung Research
--