[PATCH 00/12] usb: Replace {v}snprintf() variants with safer alternatives

[PATCH 00/12] usb: Replace {v}snprintf() variants with safer alternatives

[Lee Jones]

This series is part of an effort to rid {v}snprintf() where possible.

For a far better description of the problem than I could author, see
Jon's write-up on LWN [1] and/or Alex's on the Kernel Self Protection
Project [1].

[0] https://lwn.net/Articles/69419/
[1] https://github.com/KSPP/linux/issues/105

Lee Jones (12):
  usb: gadget: configfs: Replace snprintf() with the safer scnprintf()
    variant
  usb: gadget: f_uac1: Replace snprintf() with the safer scnprintf()
    variant
  usb: gadget: f_uac2: Replace snprintf() with the safer scnprintf()
    variant
  usb: gadget: uvc: Replace snprintf() with the safer scnprintf()
    variant
  usb: gadget: udc: atmel: Replace snprintf() with the safer scnprintf()
    variant
  usb: cdns2: Replace snprintf() with the safer scnprintf() variant
  usb: host: max3421-hcd: Replace snprintf() with the safer scnprintf()
    variant
  usb: yurex: Replace snprintf() with the safer scnprintf() variant
  usb: mon_stat: Replace snprintf() with the safer scnprintf() variant
  usb: mon_text: Replace snprintf() with the safer scnprintf() variant
  usb: phy: twl6030: Remove snprintf() from sysfs call-backs and replace
    with sysfs_emit()
  usb: storage: Remove snprintf() from sysfs call-backs and replace with
    sysfs_emit()

 drivers/usb/gadget/configfs.c              |  11 +-
 drivers/usb/gadget/function/f_uac1.c       |   6 +-
 drivers/usb/gadget/function/f_uac2.c       |   6 +-
 drivers/usb/gadget/function/uvc_configfs.c |   2 +-
 drivers/usb/gadget/udc/atmel_usba_udc.c    |   3 +-
 drivers/usb/gadget/udc/cdns2/cdns2-debug.h | 138 ++++++++++-----------
 drivers/usb/host/max3421-hcd.c             |  18 +--
 drivers/usb/misc/yurex.c                   |  12 +-
 drivers/usb/mon/mon_stat.c                 |   6 +-
 drivers/usb/mon/mon_text.c                 |  28 +----
 drivers/usb/phy/phy-twl6030-usb.c          |   8 +-
 drivers/usb/storage/sierra_ms.c            |  16 +--
 12 files changed, 120 insertions(+), 134 deletions(-)

Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Andrzej Pietrasiewicz <andrzejtp2010@gmail.com>
Cc: Bryan Wu <cooloney@kernel.org>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Cristian Birsan <cristian.birsan@microchip.com>
Cc: Daniel Scally <dan.scally@ideasonboard.com>
Cc: Hema HK <hemahk@ti.com>
Cc: James Gruber <jimmyjgruber@gmail.com>
Cc: Jaswinder Singh <jaswinder.singh@linaro.org>
Cc: Julian Scheel <julian@jusst.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Pawel Laszczak <pawell@cadence.com>
Cc: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Cc: Tomoki Sekiyama <tomoki.sekiyama@gmail.com>
Cc: usb-storage@lists.one-eyed-alien.net
Cc: Yadwinder Singh <yadi.brar01@gmail.com>
-- 
2.43.0.472.g3155946c3a-goog

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

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/gadget/configfs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index b7d2a1313a684..ce3cfa1f36f51 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -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");
-- 
2.43.0.472.g3155946c3a-goog

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

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Cc: Julian Scheel <julian@jusst.de>
Cc: Bryan Wu <cooloney@kernel.org>
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/gadget/function/f_uac1.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac1.c b/drivers/usb/gadget/function/f_uac1.c
index 6f0e1d803dc24..998e2331effb0 100644
--- a/drivers/usb/gadget/function/f_uac1.c
+++ b/drivers/usb/gadget/function/f_uac1.c
@@ -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;
 }
-- 
2.43.0.472.g3155946c3a-goog

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

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: James Gruber <jimmyjgruber@gmail.com>
Cc: Yadwinder Singh <yadi.brar01@gmail.com>
Cc: Jaswinder Singh <jaswinder.singh@linaro.org>
Cc: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/gadget/function/f_uac2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index f9a0f07a7476b..383f6854cfec5 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -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;
-- 
2.43.0.472.g3155946c3a-goog

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

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Daniel Scally <dan.scally@ideasonboard.com>
Cc: Andrzej Pietrasiewicz <andrzejtp2010@gmail.com>
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/gadget/function/uvc_configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 9bf0e985acfab..7e704b2bcfd1c 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -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;							\
-- 
2.43.0.472.g3155946c3a-goog

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

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: Cristian Birsan <cristian.birsan@microchip.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 02b1bef5e22e2..b76885d78e8a7 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -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;
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 06/12] usb: cdns2: Replace snprintf() with the safer scnprintf() variant

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: Pawel Laszczak <pawell@cadence.com>
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/gadget/udc/cdns2/cdns2-debug.h | 138 ++++++++++-----------
 1 file changed, 69 insertions(+), 69 deletions(-)

diff --git a/drivers/usb/gadget/udc/cdns2/cdns2-debug.h b/drivers/usb/gadget/udc/cdns2/cdns2-debug.h
index be9ae0d28a409..f5f330004190e 100644
--- a/drivers/usb/gadget/udc/cdns2/cdns2-debug.h
+++ b/drivers/usb/gadget/udc/cdns2/cdns2-debug.h
@@ -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;
 }
-- 
2.43.0.472.g3155946c3a-goog

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

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: Cristian Birsan <cristian.birsan@microchip.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/host/max3421-hcd.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index d152d72de1269..9fe4f48b18980 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -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;
 			}
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 08/12] usb: yurex: Replace snprintf() with the safer scnprintf() variant

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

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: Tomoki Sekiyama <tomoki.sekiyama@gmail.com>
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/misc/yurex.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/misc/yurex.c b/drivers/usb/misc/yurex.c
index c640f98d20c54..5a13cddace0e6 100644
--- a/drivers/usb/misc/yurex.c
+++ b/drivers/usb/misc/yurex.c
@@ -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);
 }
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH 09/12] usb: mon_stat: Replace snprintf() with the safer scnprintf() variant

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/mon/mon_stat.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/mon/mon_stat.c b/drivers/usb/mon/mon_stat.c
index 98ab0cc473d67..3c23805ab1a44 100644
--- a/drivers/usb/mon/mon_stat.c
+++ b/drivers/usb/mon/mon_stat.c
@@ -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;
-- 
2.43.0.472.g3155946c3a-goog

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

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

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/mon/mon_text.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/mon/mon_text.c b/drivers/usb/mon/mon_text.c
index 39cb141646526..2fe9b95bac1d5 100644
--- a/drivers/usb/mon/mon_text.c
+++ b/drivers/usb/mon/mon_text.c
@@ -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)
-- 
2.43.0.472.g3155946c3a-goog

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

[Lee Jones]

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.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: Hema HK <hemahk@ti.com>
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/phy/phy-twl6030-usb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/phy/phy-twl6030-usb.c b/drivers/usb/phy/phy-twl6030-usb.c
index c3ce6b1054f1c..da09cff55abce 100644
--- a/drivers/usb/phy/phy-twl6030-usb.c
+++ b/drivers/usb/phy/phy-twl6030-usb.c
@@ -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);
 
-- 
2.43.0.472.g3155946c3a-goog

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

[Lee Jones]

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.

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: usb-storage@lists.one-eyed-alien.net
Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/storage/sierra_ms.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/storage/sierra_ms.c b/drivers/usb/storage/sierra_ms.c
index 0774ba22fb66e..177fa6cd143ab 100644
--- a/drivers/usb/storage/sierra_ms.c
+++ b/drivers/usb/storage/sierra_ms.c
@@ -98,26 +98,26 @@ static ssize_t truinst_show(struct device *dev, struct device_attribute *attr,
 	struct usb_device *udev = interface_to_usbdev(intf);
 	int result;
 	if (swi_tru_install == TRU_FORCE_MS) {
-		result = snprintf(buf, PAGE_SIZE, "Forced Mass Storage\n");
+		result = sysfs_emit(buf, "Forced Mass Storage\n");
 	} else {
 		swocInfo = kmalloc(sizeof(struct swoc_info), GFP_KERNEL);
 		if (!swocInfo) {
-			snprintf(buf, PAGE_SIZE, "Error\n");
+			sysfs_emit(buf, "Error\n");
 			return -ENOMEM;
 		}
 		result = sierra_get_swoc_info(udev, swocInfo);
 		if (result < 0) {
 			dev_dbg(dev, "SWIMS: failed SWoC query\n");
 			kfree(swocInfo);
-			snprintf(buf, PAGE_SIZE, "Error\n");
+			sysfs_emit(buf, "Error\n");
 			return -EIO;
 		}
 		debug_swoc(dev, swocInfo);
-		result = snprintf(buf, PAGE_SIZE,
-			"REV=%02d SKU=%04X VER=%04X\n",
-			swocInfo->rev,
-			swocInfo->LinuxSKU,
-			swocInfo->LinuxVer);
+		result = sysfs_emit(buf,
+				    "REV=%02d SKU=%04X VER=%04X\n",
+				    swocInfo->rev,
+				    swocInfo->LinuxSKU,
+				    swocInfo->LinuxVer);
 		kfree(swocInfo);
 	}
 	return result;
-- 
2.43.0.472.g3155946c3a-goog

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

[Laurent Pinchart]

Hi Lee,

Thank you for the patch.

On Wed, Dec 13, 2023 at 04:42:33PM +0000, Lee Jones wrote:
> 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.
> 
> Link: https://lwn.net/Articles/69419/
> Link: https://github.com/KSPP/linux/issues/105
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Daniel Scally <dan.scally@ideasonboard.com>
> Cc: Andrzej Pietrasiewicz <andrzejtp2010@gmail.com>
> Signed-off-by: Lee Jones <lee@kernel.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/usb/gadget/function/uvc_configfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 9bf0e985acfab..7e704b2bcfd1c 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -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;							\

-- 
Regards,

Laurent Pinchart

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

[Alan Stern]

On Wed, Dec 13, 2023 at 04:42:41PM +0000, Lee Jones wrote:
> 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.
> 
> Link: https://lwn.net/Articles/69419/
> Link: https://github.com/KSPP/linux/issues/105
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: usb-storage@lists.one-eyed-alien.net
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>