gadget: f_thor: fix filename overflow

PATCH New Seung-Woo Kim 2 participants 4 messages May 10, 2018 – May 11, 2018 1 message not in the archive USB

Share & tools

A message that is not in the archive (sent off-list or not yet received): CGME20180510015213epcas1p1ecf6315ed1e762c0d51338137c658425@epcas1p1.samsung.com

Seung-Woo Kim

author

2018-05-10 01:52 UTC #1 lore raw

PATCH 1/2 gadget: f_thor: fix filename overflow

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 <[email protected]>

---
 drivers/usb/gadget/f_thor.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/usb/gadget/f_thor.c +2 -1

index f874509..6d38cb6 100644
@@ -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);

Signature · 1 line

1.7.9.5

Seung-Woo Kim

author

2018-05-10 01:52 UTC #2 lore raw

PATCH 2/2 gadget: f_thor: update to support more than 4GB file as thor 5.0

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 <[email protected]>

---
 drivers/usb/gadget/f_thor.c |   10 +++++++---
 drivers/usb/gadget/f_thor.h |    2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

drivers/usb/gadget/f_thor.c +7 -3

index 6d38cb6..c8eda05 100644
@@ -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';

drivers/usb/gadget/f_thor.h +1 -1

index 47abc8a..8ba3fa2 100644
@@ -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 {

Signature · 1 line

1.7.9.5

Lukasz Majewski

2018-05-10 10:58 UTC #3 lore raw

PATCH 2/2 gadget: f_thor: update to support more than 4GB file as thor 5.0

Hi Seung-Woo,

Quoted 4 lines

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?

Quoted 44 lines

Signed-off-by Seung-Woo Kim <[email protected]>

---
 drivers/usb/gadget/f_thor.c |   10 +++++++---
 drivers/usb/gadget/f_thor.h |    2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

drivers/usb/gadget/f_thor.c +1 -2

index 6d38cb6..c8eda05 100644
@@ -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] +

drivers/usb/gadget/f_thor.c +1 -0

+				 (((unsigned long long

int)rqt->int_data[1])

drivers/usb/gadget/f_thor.c +3 -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] +

drivers/usb/gadget/f_thor.c +1 -0

+				 (((unsigned long long

int)rqt->int_data[2])

drivers/usb/gadget/f_thor.c +1 -0

+				  << 32);
 		memcpy(f_name, rqt->str_data[0], F_NAME_BUF_SIZE);
 		f_name[F_NAME_BUF_SIZE] = '\0';

drivers/usb/gadget/f_thor.h +0 -0

index 47abc8a..8ba3fa2 100644

drivers/usb/gadget/f_thor.h struct usb_cdc_attribute_vendor_descriptor { · line 40

Quoted 8 lines

drivers/usb/gadget/f_thor.h +1 -1

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

Seung-Woo Kim

author

2018-05-11 00:48 UTC #4 lore raw

PATCH 2/2 gadget: f_thor: update to support more than 4GB file as thor 5.0

Hello Lukasz,

Quoted 10 lines · Lukasz Majewski

Hi Seung-Woo,
Quoted 4 lines
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.

Quoted 3 lines

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

Quoted 64 lines

Quoted 45 lines
Signed-off-by Seung-Woo Kim <[email protected]>
---
 drivers/usb/gadget/f_thor.c |   10 +++++++---
 drivers/usb/gadget/f_thor.h |    2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)
drivers/usb/gadget/f_thor.c +1 -2
index 6d38cb6..c8eda05 100644
@@ -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] +
drivers/usb/gadget/f_thor.c +1 -0
+				 (((unsigned long long
int)rqt->int_data[1])
drivers/usb/gadget/f_thor.c +3 -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] +
drivers/usb/gadget/f_thor.c +1 -0
+				 (((unsigned long long
int)rqt->int_data[2])
drivers/usb/gadget/f_thor.c +1 -0
+				  << 32);
 		memcpy(f_name, rqt->str_data[0], F_NAME_BUF_SIZE);
 		f_name[F_NAME_BUF_SIZE] = '\0';
drivers/usb/gadget/f_thor.h +0 -0
index 47abc8a..8ba3fa2 100644
drivers/usb/gadget/f_thor.h struct usb_cdc_attribute_vendor_descriptor { · line 40
Quoted 8 lines
drivers/usb/gadget/f_thor.h +1 -1
@@ -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

Signature · 3 lines

Seung-Woo Kim
Samsung Research
--