usb: Replace {v}snprintf() variants with safer alternatives

Cc Nicolas Ferre <[email protected]>

Cc Pawel Laszczak <[email protected]>

Cc Ruslan Bilovol <[email protected]>

Cc Tomoki Sekiyama <[email protected]>

Cc Yadwinder Singh <[email protected]>

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:42 UTC #1 lore raw

PATCH 1/12 usb: gadget: configfs: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/configfs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

drivers/usb/gadget/configfs.c +7 -4

index b7d2a1313a684..ce3cfa1f36f51 100644
@@ -606,10 +606,11 @@ static struct config_group *function_make(
 	char *instance_name;
 	int ret;
 
-	ret = snprintf(buf, MAX_NAME_LEN, "%s", name);
-	if (ret >= MAX_NAME_LEN)
+	if (strlen(name) >= MAX_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
 
+	scnprintf(buf, MAX_NAME_LEN, "%s", name);
+
 	func_name = buf;
 	instance_name = strchr(func_name, '.');
 	if (!instance_name) {

@@ -701,10 +702,12 @@ static struct config_group *config_desc_make(
 	int ret;
 
 	gi = container_of(group, struct gadget_info, configs_group);
-	ret = snprintf(buf, MAX_NAME_LEN, "%s", name);
-	if (ret >= MAX_NAME_LEN)
+
+	if (strlen(name) >= MAX_NAME_LEN)
 		return ERR_PTR(-ENAMETOOLONG);
 
+	scnprintf(buf, MAX_NAME_LEN, "%s", name);
+
 	num_str = strchr(buf, '.');
 	if (!num_str) {
 		pr_err("Unable to locate . in name.bConfigurationValue\n");

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #2 lore raw

PATCH 2/12 usb: gadget: f_uac1: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Cc Ruslan Bilovol <[email protected]>

Cc Julian Scheel <[email protected]>

Cc Bryan Wu <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/function/f_uac1.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/usb/gadget/function/f_uac1.c +3 -3

index 6f0e1d803dc24..998e2331effb0 100644
@@ -1561,7 +1561,7 @@ static ssize_t f_uac1_opts_##name##_show(struct config_item *item,	\
 	int result;							\
 									\
 	mutex_lock(&opts->lock);					\
-	result = snprintf(page, sizeof(opts->name), "%s", opts->name);	\
+	result = scnprintf(page, sizeof(opts->name), "%s", opts->name);	\
 	mutex_unlock(&opts->lock);					\
 									\
 	return result;							\

@@ -1579,7 +1579,7 @@ static ssize_t f_uac1_opts_##name##_store(struct config_item *item,	\
 		goto end;						\
 	}								\
 									\
-	ret = snprintf(opts->name, min(sizeof(opts->name), len),	\
+	ret = scnprintf(opts->name, min(sizeof(opts->name), len),	\
 			"%s", page);					\
 									\
 end:									\

@@ -1685,7 +1685,7 @@ static struct usb_function_instance *f_audio_alloc_inst(void)
 
 	opts->req_number = UAC1_DEF_REQ_NUM;
 
-	snprintf(opts->function_name, sizeof(opts->function_name), "AC Interface");
+	scnprintf(opts->function_name, sizeof(opts->function_name), "AC Interface");
 
 	return &opts->func_inst;
 }

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #3 lore raw

PATCH 3/12 usb: gadget: f_uac2: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Cc James Gruber <[email protected]>

Cc Yadwinder Singh <[email protected]>

Cc Jaswinder Singh <[email protected]>

Cc Ruslan Bilovol <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/function/f_uac2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/usb/gadget/function/f_uac2.c +3 -3

index f9a0f07a7476b..383f6854cfec5 100644
@@ -2045,7 +2045,7 @@ static ssize_t f_uac2_opts_##name##_show(struct config_item *item,	\
 	int result;							\
 									\
 	mutex_lock(&opts->lock);					\
-	result = snprintf(page, sizeof(opts->name), "%s", opts->name);	\
+	result = scnprintf(page, sizeof(opts->name), "%s", opts->name);	\
 	mutex_unlock(&opts->lock);					\
 									\
 	return result;							\

@@ -2063,7 +2063,7 @@ static ssize_t f_uac2_opts_##name##_store(struct config_item *item,	\
 		goto end;						\
 	}								\
 									\
-	ret = snprintf(opts->name, min(sizeof(opts->name), len),	\
+	ret = scnprintf(opts->name, min(sizeof(opts->name), len),	\
 			"%s", page);					\
 									\
 end:									\

@@ -2187,7 +2187,7 @@ static struct usb_function_instance *afunc_alloc_inst(void)
 	opts->req_number = UAC2_DEF_REQ_NUM;
 	opts->fb_max = FBACK_FAST_MAX;
 
-	snprintf(opts->function_name, sizeof(opts->function_name), "Source/Sink");
+	scnprintf(opts->function_name, sizeof(opts->function_name), "Source/Sink");
 
 	opts->p_terminal_type = UAC2_DEF_P_TERM_TYPE;
 	opts->c_terminal_type = UAC2_DEF_C_TERM_TYPE;

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #4 lore raw

PATCH 4/12 usb: gadget: uvc: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Cc Laurent Pinchart <[email protected]>

Cc Daniel Scally <[email protected]>

Cc Andrzej Pietrasiewicz <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/function/uvc_configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/gadget/function/uvc_configfs.c +1 -1

index 9bf0e985acfab..7e704b2bcfd1c 100644
@@ -3414,7 +3414,7 @@ static ssize_t f_uvc_opts_string_##cname##_show(struct config_item *item,\
 	int result;							\
 									\
 	mutex_lock(&opts->lock);					\
-	result = snprintf(page, sizeof(opts->aname), "%s", opts->aname);\
+	result = scnprintf(page, sizeof(opts->aname), "%s", opts->aname);\
 	mutex_unlock(&opts->lock);					\
 									\
 	return result;							\

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Laurent Pinchart

Reviewed-by on v1 4/12

2023-12-13 16:47 UTC #5 lore raw

Re: [PATCH 04/12] usb: gadget: uvc: Replace snprintf() with the safer scnprintf() variant

Hi Lee,

Thank you for the patch.

Quoted 9 lines · Lee Jones

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Quoted 6 lines

Link https://lwn.net/Articles/69419/
Link https://github.com/KSPP/linux/issues/105
Cc Laurent Pinchart <[email protected]>
Cc Daniel Scally <[email protected]>
Cc Andrzej Pietrasiewicz <[email protected]>
Signed-off-by Lee Jones <[email protected]>

Reviewed-by Laurent Pinchart <[email protected]>

Quoted 17 lines

---
 drivers/usb/gadget/function/uvc_configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/gadget/function/uvc_configfs.c +1 -1

index 9bf0e985acfab..7e704b2bcfd1c 100644
@@ -3414,7 +3414,7 @@ static ssize_t f_uvc_opts_string_##cname##_show(struct config_item *item,\
 	int result;							\
 									\
 	mutex_lock(&opts->lock);					\
-	result = snprintf(page, sizeof(opts->aname), "%s", opts->aname);\
+	result = scnprintf(page, sizeof(opts->aname), "%s", opts->aname);\
 	mutex_unlock(&opts->lock);					\
 									\
 	return result;							\

Signature · 3 lines

Regards,

Laurent Pinchart

Lee Jones

author

2023-12-13 16:43 UTC #6 lore raw

PATCH 5/12 usb: gadget: udc: atmel: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Cc Cristian Birsan <[email protected]>

Cc Nicolas Ferre <[email protected]>

Cc Alexandre Belloni <[email protected]>

Cc Claudiu Beznea <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/usb/gadget/udc/atmel_usba_udc.c +1 -2

index 02b1bef5e22e2..b76885d78e8a7 100644
@@ -94,7 +94,7 @@ static ssize_t queue_dbg_read(struct file *file, char __user *buf,
 
 	inode_lock(file_inode(file));
 	list_for_each_entry_safe(req, tmp_req, queue, queue) {
-		len = snprintf(tmpbuf, sizeof(tmpbuf),
+		len = scnprintf(tmpbuf, sizeof(tmpbuf),
 				"%8p %08x %c%c%c %5d %c%c%c\n",
 				req->req.buf, req->req.length,
 				req->req.no_interrupt ? 'i' : 'I',

@@ -104,7 +104,6 @@ static ssize_t queue_dbg_read(struct file *file, char __user *buf,
 				req->submitted ? 'F' : 'f',
 				req->using_dma ? 'D' : 'd',
 				req->last_transaction ? 'L' : 'l');
-		len = min(len, sizeof(tmpbuf));
 		if (len > nbytes)
 			break;

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #7 lore raw

PATCH 6/12 usb: cdns2: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Cc Pawel Laszczak <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/udc/cdns2/cdns2-debug.h | 138 ++++++++++-----------
 1 file changed, 69 insertions(+), 69 deletions(-)

drivers/usb/gadget/udc/cdns2/cdns2-debug.h +69 ~~-69~~

index be9ae0d28a409..f5f330004190e 100644
@@ -16,34 +16,34 @@ static inline const char *cdns2_decode_usb_irq(char *str, size_t size,
 {
 	int ret;
 
-	ret = snprintf(str, size, "usbirq: 0x%02x - ", usb_irq);
+	ret = scnprintf(str, size, "usbirq: 0x%02x - ", usb_irq);
 
 	if (usb_irq & USBIRQ_SOF)
-		ret += snprintf(str + ret, size - ret, "SOF ");
+		ret += scnprintf(str + ret, size - ret, "SOF ");
 	if (usb_irq & USBIRQ_SUTOK)
-		ret += snprintf(str + ret, size - ret, "SUTOK ");
+		ret += scnprintf(str + ret, size - ret, "SUTOK ");
 	if (usb_irq & USBIRQ_SUDAV)
-		ret += snprintf(str + ret, size - ret, "SETUP ");
+		ret += scnprintf(str + ret, size - ret, "SETUP ");
 	if (usb_irq & USBIRQ_SUSPEND)
-		ret += snprintf(str + ret, size - ret, "Suspend ");
+		ret += scnprintf(str + ret, size - ret, "Suspend ");
 	if (usb_irq & USBIRQ_URESET)
-		ret += snprintf(str + ret, size - ret, "Reset ");
+		ret += scnprintf(str + ret, size - ret, "Reset ");
 	if (usb_irq & USBIRQ_HSPEED)
-		ret += snprintf(str + ret, size - ret, "HS ");
+		ret += scnprintf(str + ret, size - ret, "HS ");
 	if (usb_irq & USBIRQ_LPM)
-		ret += snprintf(str + ret, size - ret, "LPM ");
+		ret += scnprintf(str + ret, size - ret, "LPM ");
 
-	ret += snprintf(str + ret, size - ret, ", EXT: 0x%02x - ", ext_irq);
+	ret += scnprintf(str + ret, size - ret, ", EXT: 0x%02x - ", ext_irq);
 
 	if (ext_irq & EXTIRQ_WAKEUP)
-		ret += snprintf(str + ret, size - ret, "Wakeup ");
+		ret += scnprintf(str + ret, size - ret, "Wakeup ");
 	if (ext_irq & EXTIRQ_VBUSFAULT_FALL)
-		ret += snprintf(str + ret, size - ret, "VBUS_FALL ");
+		ret += scnprintf(str + ret, size - ret, "VBUS_FALL ");
 	if (ext_irq & EXTIRQ_VBUSFAULT_RISE)
-		ret += snprintf(str + ret, size - ret, "VBUS_RISE ");
+		ret += scnprintf(str + ret, size - ret, "VBUS_RISE ");
 
-	if (ret >= size)
-		pr_info("CDNS2: buffer overflowed.\n");
+	if (ret == size - 1)
+		pr_info("CDNS2: buffer may be truncated.\n");
 
 	return str;
 }

@@ -54,28 +54,28 @@ static inline const char *cdns2_decode_dma_irq(char *str, size_t size,
 {
 	int ret;
 
-	ret = snprintf(str, size, "ISTS: %08x, %s: %08x ",
-		       ep_ists, ep_name, ep_sts);
+	ret = scnprintf(str, size, "ISTS: %08x, %s: %08x ",
+			ep_ists, ep_name, ep_sts);
 
 	if (ep_sts & DMA_EP_STS_IOC)
-		ret += snprintf(str + ret, size - ret, "IOC ");
+		ret += scnprintf(str + ret, size - ret, "IOC ");
 	if (ep_sts & DMA_EP_STS_ISP)
-		ret += snprintf(str + ret, size - ret, "ISP ");
+		ret += scnprintf(str + ret, size - ret, "ISP ");
 	if (ep_sts & DMA_EP_STS_DESCMIS)
-		ret += snprintf(str + ret, size - ret, "DESCMIS ");
+		ret += scnprintf(str + ret, size - ret, "DESCMIS ");
 	if (ep_sts & DMA_EP_STS_TRBERR)
-		ret += snprintf(str + ret, size - ret, "TRBERR ");
+		ret += scnprintf(str + ret, size - ret, "TRBERR ");
 	if (ep_sts & DMA_EP_STS_OUTSMM)
-		ret += snprintf(str + ret, size - ret, "OUTSMM ");
+		ret += scnprintf(str + ret, size - ret, "OUTSMM ");
 	if (ep_sts & DMA_EP_STS_ISOERR)
-		ret += snprintf(str + ret, size - ret, "ISOERR ");
+		ret += scnprintf(str + ret, size - ret, "ISOERR ");
 	if (ep_sts & DMA_EP_STS_DBUSY)
-		ret += snprintf(str + ret, size - ret, "DBUSY ");
+		ret += scnprintf(str + ret, size - ret, "DBUSY ");
 	if (DMA_EP_STS_CCS(ep_sts))
-		ret += snprintf(str + ret, size - ret, "CCS ");
+		ret += scnprintf(str + ret, size - ret, "CCS ");
 
-	if (ret >= size)
-		pr_info("CDNS2: buffer overflowed.\n");
+	if (ret == size - 1)
+		pr_info("CDNS2: buffer may be truncated.\n");
 
 	return str;
 }

@@ -105,43 +105,43 @@ static inline const char *cdns2_raw_ring(struct cdns2_endpoint *pep,
 	int ret;
 	int i;
 
-	ret = snprintf(str, size, "\n\t\tTR for %s:", pep->name);
+	ret = scnprintf(str, size, "\n\t\tTR for %s:", pep->name);
 
 	trb = &trbs[ring->dequeue];
 	dma = cdns2_trb_virt_to_dma(pep, trb);
-	ret += snprintf(str + ret, size - ret,
-			"\n\t\tRing deq index: %d, trb: V=%p, P=0x%pad\n",
-			ring->dequeue, trb, &dma);
+	ret += scnprintf(str + ret, size - ret,
+			 "\n\t\tRing deq index: %d, trb: V=%p, P=0x%pad\n",
+			 ring->dequeue, trb, &dma);
 
 	trb = &trbs[ring->enqueue];
 	dma = cdns2_trb_virt_to_dma(pep, trb);
-	ret += snprintf(str + ret, size - ret,
-			"\t\tRing enq index: %d, trb: V=%p, P=0x%pad\n",
-			ring->enqueue, trb, &dma);
+	ret += scnprintf(str + ret, size - ret,
+			 "\t\tRing enq index: %d, trb: V=%p, P=0x%pad\n",
+			 ring->enqueue, trb, &dma);
 
-	ret += snprintf(str + ret, size - ret,
-			"\t\tfree trbs: %d, CCS=%d, PCS=%d\n",
-			ring->free_trbs, ring->ccs, ring->pcs);
+	ret += scnprintf(str + ret, size - ret,
+			 "\t\tfree trbs: %d, CCS=%d, PCS=%d\n",
+			 ring->free_trbs, ring->ccs, ring->pcs);
 
 	if (TRBS_PER_SEGMENT > 40) {
-		ret += snprintf(str + ret, size - ret,
-				"\t\tTransfer ring %d too big\n", TRBS_PER_SEGMENT);
+		ret += scnprintf(str + ret, size - ret,
+				 "\t\tTransfer ring %d too big\n", TRBS_PER_SEGMENT);
 		return str;
 	}
 
 	dma = ring->dma;
 	for (i = 0; i < TRBS_PER_SEGMENT; ++i) {
 		trb = &trbs[i];
-		ret += snprintf(str + ret, size - ret,
-				"\t\t@%pad %08x %08x %08x\n", &dma,
-				le32_to_cpu(trb->buffer),
-				le32_to_cpu(trb->length),
-				le32_to_cpu(trb->control));
+		ret += scnprintf(str + ret, size - ret,
+				 "\t\t@%pad %08x %08x %08x\n", &dma,
+				 le32_to_cpu(trb->buffer),
+				 le32_to_cpu(trb->length),
+				 le32_to_cpu(trb->control));
 		dma += sizeof(*trb);
 	}
 
-	if (ret >= size)
-		pr_info("CDNS2: buffer overflowed.\n");
+	if (ret == size - 1)
+		pr_info("CDNS2: buffer may be truncated.\n");
 
 	return str;
 }

@@ -166,36 +166,36 @@ static inline const char *cdns2_decode_trb(char *str, size_t size, u32 flags,
 
 	switch (type) {
 	case TRB_LINK:
-		ret = snprintf(str, size,
-			       "LINK %08x type '%s' flags %c:%c:%c%c:%c",
-			       buffer, cdns2_trb_type_string(type),
-			       flags & TRB_CYCLE ? 'C' : 'c',
-			       flags & TRB_TOGGLE ? 'T' : 't',
-			       flags & TRB_CHAIN ? 'C' : 'c',
-			       flags & TRB_CHAIN ? 'H' : 'h',
-			       flags & TRB_IOC ? 'I' : 'i');
+		ret = scnprintf(str, size,
+				"LINK %08x type '%s' flags %c:%c:%c%c:%c",
+				buffer, cdns2_trb_type_string(type),
+				flags & TRB_CYCLE ? 'C' : 'c',
+				flags & TRB_TOGGLE ? 'T' : 't',
+				flags & TRB_CHAIN ? 'C' : 'c',
+				flags & TRB_CHAIN ? 'H' : 'h',
+				flags & TRB_IOC ? 'I' : 'i');
 		break;
 	case TRB_NORMAL:
-		ret = snprintf(str, size,
-			       "type: '%s', Buffer: %08x, length: %ld, burst len: %ld, "
-			       "flags %c:%c:%c%c:%c",
-			       cdns2_trb_type_string(type),
-			       buffer, TRB_LEN(length),
-			       TRB_FIELD_TO_BURST(length),
-			       flags & TRB_CYCLE ? 'C' : 'c',
-			       flags & TRB_ISP ? 'I' : 'i',
-			       flags & TRB_CHAIN ? 'C' : 'c',
-			       flags & TRB_CHAIN ? 'H' : 'h',
-			       flags & TRB_IOC ? 'I' : 'i');
+		ret = scnprintf(str, size,
+				"type: '%s', Buffer: %08x, length: %ld, burst len: %ld, "
+				"flags %c:%c:%c%c:%c",
+				cdns2_trb_type_string(type),
+				buffer, TRB_LEN(length),
+				TRB_FIELD_TO_BURST(length),
+				flags & TRB_CYCLE ? 'C' : 'c',
+				flags & TRB_ISP ? 'I' : 'i',
+				flags & TRB_CHAIN ? 'C' : 'c',
+				flags & TRB_CHAIN ? 'H' : 'h',
+				flags & TRB_IOC ? 'I' : 'i');
 		break;
 	default:
-		ret = snprintf(str, size, "type '%s' -> raw %08x %08x %08x",
-			       cdns2_trb_type_string(type),
-			       buffer, length, flags);
+		ret = scnprintf(str, size, "type '%s' -> raw %08x %08x %08x",
+				cdns2_trb_type_string(type),
+				buffer, length, flags);
 	}
 
-	if (ret >= size)
-		pr_info("CDNS2: buffer overflowed.\n");
+	if (ret == size - 1)
+		pr_info("CDNS2: buffer may be truncated.\n");
 
 	return str;
 }

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #8 lore raw

PATCH 7/12 usb: host: max3421-hcd: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Cc Cristian Birsan <[email protected]>

Cc Nicolas Ferre <[email protected]>

Cc Alexandre Belloni <[email protected]>

Cc Claudiu Beznea <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/host/max3421-hcd.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

drivers/usb/host/max3421-hcd.c +9 -9

index d152d72de1269..9fe4f48b18980 100644
@@ -1158,12 +1158,12 @@ dump_eps(struct usb_hcd *hcd)
 		end = dp + sizeof(ubuf);
 		*dp = '\0';
 		list_for_each_entry(urb, &ep->urb_list, urb_list) {
-			ret = snprintf(dp, end - dp, " %p(%d.%s %d/%d)", urb,
-				       usb_pipetype(urb->pipe),
-				       usb_urb_dir_in(urb) ? "IN" : "OUT",
-				       urb->actual_length,
-				       urb->transfer_buffer_length);
-			if (ret < 0 || ret >= end - dp)
+			ret = scnprintf(dp, end - dp, " %p(%d.%s %d/%d)", urb,
+					usb_pipetype(urb->pipe),
+					usb_urb_dir_in(urb) ? "IN" : "OUT",
+					urb->actual_length,
+					urb->transfer_buffer_length);
+			if (ret == end - dp - 1)
 				break;	/* error or buffer full */
 			dp += ret;
 		}

@@ -1255,9 +1255,9 @@ max3421_handle_irqs(struct usb_hcd *hcd)
 			end = sbuf + sizeof(sbuf);
 			*dp = '\0';
 			for (i = 0; i < 16; ++i) {
-				int ret = snprintf(dp, end - dp, " %lu",
-						   max3421_hcd->err_stat[i]);
-				if (ret < 0 || ret >= end - dp)
+				int ret = scnprintf(dp, end - dp, " %lu",
+						    max3421_hcd->err_stat[i]);
+				if (ret == end - dp - 1)
 					break;	/* error or buffer full */
 				dp += ret;
 			}

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #9 lore raw

PATCH 8/12 usb: yurex: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Whilst we're at it, let's define some magic numbers to increase readability and ease of maintenance.

Cc Tomoki Sekiyama <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/misc/yurex.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/usb/misc/yurex.c +7 -5

index c640f98d20c54..5a13cddace0e6 100644
@@ -34,6 +34,8 @@
 #define YUREX_BUF_SIZE		8
 #define YUREX_WRITE_TIMEOUT	(HZ*2)
 
+#define MAX_S64_STRLEN 20 /* {-}922337203685477580{7,8} */
+
 /* table of devices that work with this driver */
 static struct usb_device_id yurex_table[] = {
 	{ USB_DEVICE(YUREX_VENDOR_ID, YUREX_PRODUCT_ID) },

@@ -401,7 +403,7 @@ static ssize_t yurex_read(struct file *file, char __user *buffer, size_t count,
 {
 	struct usb_yurex *dev;
 	int len = 0;
-	char in_buffer[20];
+	char in_buffer[MAX_S64_STRLEN];
 	unsigned long flags;
 
 	dev = file->private_data;

@@ -412,14 +414,14 @@ static ssize_t yurex_read(struct file *file, char __user *buffer, size_t count,
 		return -ENODEV;
 	}
 
+	if (WARN_ON_ONCE(dev->bbu > S64_MAX || dev->bbu < S64_MIN))
+		return -EIO;
+
 	spin_lock_irqsave(&dev->lock, flags);
-	len = snprintf(in_buffer, 20, "%lld\n", dev->bbu);
+	scnprintf(in_buffer, MAX_S64_STRLEN, "%lld\n", dev->bbu);
 	spin_unlock_irqrestore(&dev->lock, flags);
 	mutex_unlock(&dev->io_mutex);
 
-	if (WARN_ON_ONCE(len >= sizeof(in_buffer)))
-		return -EIO;
-
 	return simple_read_from_buffer(buffer, count, ppos, in_buffer, len);
 }

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #10 lore raw

PATCH 9/12 usb: mon_stat: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/mon/mon_stat.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/usb/mon/mon_stat.c +3 -3

index 98ab0cc473d67..3c23805ab1a44 100644
@@ -35,9 +35,9 @@ static int mon_stat_open(struct inode *inode, struct file *file)
 
 	mbus = inode->i_private;
 
-	sp->slen = snprintf(sp->str, STAT_BUF_SIZE,
-	    "nreaders %d events %u text_lost %u\n",
-	    mbus->nreaders, mbus->cnt_events, mbus->cnt_text_lost);
+	sp->slen = scnprintf(sp->str, STAT_BUF_SIZE,
+			     "nreaders %d events %u text_lost %u\n",
+			     mbus->nreaders, mbus->cnt_events, mbus->cnt_text_lost);
 
 	file->private_data = sp;
 	return 0;

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #11 lore raw

PATCH 10/12 usb: mon_text: Replace snprintf() with the safer scnprintf() variant

There is a general misunderstanding amongst engineers that {v}snprintf() returns the length of the data *actually* encoded into the destination array. However, as per the C99 standard {v}snprintf() really returns the length of the data that *would have been* written if there were enough space for it. This misunderstanding has led to buffer-overruns in the past. It's generally considered safer to use the {v}scnprintf() variants in their place (or even sprintf() in simple cases). So let's do that.

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/mon/mon_text.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

drivers/usb/mon/mon_text.c +5 ~~-23~~

index 39cb141646526..2fe9b95bac1d5 100644
@@ -352,7 +352,7 @@ static int mon_text_open(struct inode *inode, struct file *file)
 	rp->r.rnf_error = mon_text_error;
 	rp->r.rnf_complete = mon_text_complete;
 
-	snprintf(rp->slab_name, SLAB_NAME_SZ, "mon_text_%p", rp);
+	scnprintf(rp->slab_name, SLAB_NAME_SZ, "mon_text_%p", rp);
 	rp->e_slab = kmem_cache_create(rp->slab_name,
 	    sizeof(struct mon_event_text), sizeof(long), 0,
 	    mon_text_ctor);

@@ -700,46 +700,28 @@ static const struct file_operations mon_fops_text_u = {
 
 int mon_text_add(struct mon_bus *mbus, const struct usb_bus *ubus)
 {
-	enum { NAMESZ = 10 };
+	enum { NAMESZ = 12 };
 	char name[NAMESZ];
 	int busnum = ubus? ubus->busnum: 0;
-	int rc;
 
 	if (mon_dir == NULL)
 		return 0;
 
 	if (ubus != NULL) {
-		rc = snprintf(name, NAMESZ, "%dt", busnum);
-		if (rc <= 0 || rc >= NAMESZ)
-			goto err_print_t;
+		scnprintf(name, NAMESZ, "%dt", busnum);
 		mbus->dent_t = debugfs_create_file(name, 0600, mon_dir, mbus,
 							     &mon_fops_text_t);
 	}
 
-	rc = snprintf(name, NAMESZ, "%du", busnum);
-	if (rc <= 0 || rc >= NAMESZ)
-		goto err_print_u;
+	scnprintf(name, NAMESZ, "%du", busnum);
 	mbus->dent_u = debugfs_create_file(name, 0600, mon_dir, mbus,
 					   &mon_fops_text_u);
 
-	rc = snprintf(name, NAMESZ, "%ds", busnum);
-	if (rc <= 0 || rc >= NAMESZ)
-		goto err_print_s;
+	scnprintf(name, NAMESZ, "%ds", busnum);
 	mbus->dent_s = debugfs_create_file(name, 0600, mon_dir, mbus,
 					   &mon_fops_stat);
 
 	return 1;
-
-err_print_s:
-	debugfs_remove(mbus->dent_u);
-	mbus->dent_u = NULL;
-err_print_u:
-	if (ubus != NULL) {
-		debugfs_remove(mbus->dent_t);
-		mbus->dent_t = NULL;
-	}
-err_print_t:
-	return 0;
 }
 
 void mon_text_del(struct mon_bus *mbus)

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #12 lore raw

PATCH 11/12 usb: phy: twl6030: Remove snprintf() from sysfs call-backs and replace with sysfs_emit()

Since snprintf() has the documented, but still rather strange trait of returning the length of the data that *would have been* written to the array if space were available, rather than the arguably more useful length of data *actually* written, it is usually considered wise to use something else instead in order to avoid confusion.

In the case of sysfs call-backs, new wrappers exist that do just that.

Cc Hema HK <[email protected]>

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/phy/phy-twl6030-usb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/usb/phy/phy-twl6030-usb.c +4 -4

index c3ce6b1054f1c..da09cff55abce 100644
@@ -179,16 +179,16 @@ static ssize_t vbus_show(struct device *dev,
 
 	switch (twl->linkstat) {
 	case MUSB_VBUS_VALID:
-	       ret = snprintf(buf, PAGE_SIZE, "vbus\n");
+	       ret = sysfs_emit(buf, "vbus\n");
 	       break;
 	case MUSB_ID_GROUND:
-	       ret = snprintf(buf, PAGE_SIZE, "id\n");
+	       ret = sysfs_emit(buf, "id\n");
 	       break;
 	case MUSB_VBUS_OFF:
-	       ret = snprintf(buf, PAGE_SIZE, "none\n");
+	       ret = sysfs_emit(buf, "none\n");
 	       break;
 	default:
-	       ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
+	       ret = sysfs_emit(buf, "UNKNOWN\n");
 	}
 	spin_unlock_irqrestore(&twl->lock, flags);

Signature · 1 line

2.43.0.472.g3155946c3a-goog

Lee Jones

author

2023-12-13 16:43 UTC #13 lore raw

PATCH 12/12 usb: storage: Remove snprintf() from sysfs call-backs and replace with sysfs_emit()

Since snprintf() has the documented, but still rather strange trait of returning the length of the data that *would have been* written to the array if space were available, rather than the arguably more useful length of data *actually* written, it is usually considered wise to use something else instead in order to avoid confusion.

In the case of sysfs call-backs, new wrappers exist that do just that.

Cc Alan Stern <[email protected]>