DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] staging: vhciq_core: replace snprintf with scnprintf
@ 2019-09-11 13:51 Rohit Sarkar
  2019-09-11 14:17 ` Stefan Wahren
  0 siblings, 1 reply; 7+ messages in thread
From: Rohit Sarkar @ 2019-09-11 13:51 UTC (permalink / raw)
  To: Eric Anholt, Nicolas Saenz Julienne
  Cc: Stefan Wahren, devel, Greg Kroah-Hartman, kernel-janitors,
	Tobias Büttner, linux-rpi-kernel, Dominic Braun

When the number of bytes to be printed exceeds the limit snprintf
returns the number of bytes that would have been printed (if there was
no truncation). This might cause issues, hence use scnprintf which
returns the actual number of bytes printed to buffer always

Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
---
 .../interface/vchiq_arm/vchiq_core.c          | 38 +++++++++----------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 183f5cf887e0..56a23a297fa4 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3322,13 +3322,13 @@ vchiq_dump_shared_state(void *dump_context, struct vchiq_state *state,
 	char buf[80];
 	int len;
 
-	len = snprintf(buf, sizeof(buf),
+	len = scnprintf(buf, sizeof(buf),
 		"  %s: slots %d-%d tx_pos=%x recycle=%x",
 		label, shared->slot_first, shared->slot_last,
 		shared->tx_pos, shared->slot_queue_recycle);
 	vchiq_dump(dump_context, buf, len + 1);
 
-	len = snprintf(buf, sizeof(buf),
+	len = scnprintf(buf, sizeof(buf),
 		"    Slots claimed:");
 	vchiq_dump(dump_context, buf, len + 1);
 
@@ -3336,7 +3336,7 @@ vchiq_dump_shared_state(void *dump_context, struct vchiq_state *state,
 		struct vchiq_slot_info slot_info =
 						*SLOT_INFO_FROM_INDEX(state, i);
 		if (slot_info.use_count != slot_info.release_count) {
-			len = snprintf(buf, sizeof(buf),
+			len = scnprintf(buf, sizeof(buf),
 				"      %d: %d/%d", i, slot_info.use_count,
 				slot_info.release_count);
 			vchiq_dump(dump_context, buf, len + 1);
@@ -3344,7 +3344,7 @@ vchiq_dump_shared_state(void *dump_context, struct vchiq_state *state,
 	}
 
 	for (i = 1; i < shared->debug[DEBUG_ENTRIES]; i++) {
-		len = snprintf(buf, sizeof(buf), "    DEBUG: %s = %d(%x)",
+		len = scnprintf(buf, sizeof(buf), "    DEBUG: %s = %d(%x)",
 			debug_names[i], shared->debug[i], shared->debug[i]);
 		vchiq_dump(dump_context, buf, len + 1);
 	}
@@ -3357,11 +3357,11 @@ vchiq_dump_state(void *dump_context, struct vchiq_state *state)
 	int len;
 	int i;
 
-	len = snprintf(buf, sizeof(buf), "State %d: %s", state->id,
+	len = scnprintf(buf, sizeof(buf), "State %d: %s", state->id,
 		conn_state_names[state->conn_state]);
 	vchiq_dump(dump_context, buf, len + 1);
 
-	len = snprintf(buf, sizeof(buf),
+	len = scnprintf(buf, sizeof(buf),
 		"  tx_pos=%x(@%pK), rx_pos=%x(@%pK)",
 		state->local->tx_pos,
 		state->tx_data + (state->local_tx_pos & VCHIQ_SLOT_MASK),
@@ -3369,13 +3369,13 @@ vchiq_dump_state(void *dump_context, struct vchiq_state *state)
 		state->rx_data + (state->rx_pos & VCHIQ_SLOT_MASK));
 	vchiq_dump(dump_context, buf, len + 1);
 
-	len = snprintf(buf, sizeof(buf),
+	len = scnprintf(buf, sizeof(buf),
 		"  Version: %d (min %d)",
 		VCHIQ_VERSION, VCHIQ_VERSION_MIN);
 	vchiq_dump(dump_context, buf, len + 1);
 
 	if (VCHIQ_ENABLE_STATS) {
-		len = snprintf(buf, sizeof(buf),
+		len = scnprintf(buf, sizeof(buf),
 			"  Stats: ctrl_tx_count=%d, ctrl_rx_count=%d, "
 			"error_count=%d",
 			state->stats.ctrl_tx_count, state->stats.ctrl_rx_count,
@@ -3383,7 +3383,7 @@ vchiq_dump_state(void *dump_context, struct vchiq_state *state)
 		vchiq_dump(dump_context, buf, len + 1);
 	}
 
-	len = snprintf(buf, sizeof(buf),
+	len = scnprintf(buf, sizeof(buf),
 		"  Slots: %d available (%d data), %d recyclable, %d stalls "
 		"(%d data)",
 		((state->slot_queue_available * VCHIQ_SLOT_SIZE) -
@@ -3416,7 +3416,7 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
 	char buf[80];
 	int len;
 
-	len = snprintf(buf, sizeof(buf), "Service %u: %s (ref %u)",
+	len = scnprintf(buf, sizeof(buf), "Service %u: %s (ref %u)",
 		service->localport, srvstate_names[service->srvstate],
 		service->ref_count - 1); /*Don't include the lock just taken*/
 
@@ -3428,17 +3428,17 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
 		int tx_pending, rx_pending;
 
 		if (service->remoteport != VCHIQ_PORT_FREE) {
-			int len2 = snprintf(remoteport, sizeof(remoteport),
+			int len2 = scnprintf(remoteport, sizeof(remoteport),
 				"%u", service->remoteport);
 
 			if (service->public_fourcc != VCHIQ_FOURCC_INVALID)
-				snprintf(remoteport + len2,
+				scnprintf(remoteport + len2,
 					sizeof(remoteport) - len2,
 					" (client %x)", service->client_id);
 		} else
 			strcpy(remoteport, "n/a");
 
-		len += snprintf(buf + len, sizeof(buf) - len,
+		len += scnprintf(buf + len, sizeof(buf) - len,
 			" '%c%c%c%c' remote %s (msg use %d/%d, slot use %d/%d)",
 			VCHIQ_FOURCC_AS_4CHARS(fourcc),
 			remoteport,
@@ -3455,7 +3455,7 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
 		rx_pending = service->bulk_rx.local_insert -
 			service->bulk_rx.remote_insert;
 
-		len = snprintf(buf, sizeof(buf),
+		len = scnprintf(buf, sizeof(buf),
 			"  Bulk: tx_pending=%d (size %d),"
 			" rx_pending=%d (size %d)",
 			tx_pending,
@@ -3468,7 +3468,7 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
 		if (VCHIQ_ENABLE_STATS) {
 			vchiq_dump(dump_context, buf, len + 1);
 
-			len = snprintf(buf, sizeof(buf),
+			len = scnprintf(buf, sizeof(buf),
 				"  Ctrl: tx_count=%d, tx_bytes=%llu, "
 				"rx_count=%d, rx_bytes=%llu",
 				service->stats.ctrl_tx_count,
@@ -3477,7 +3477,7 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
 				service->stats.ctrl_rx_bytes);
 			vchiq_dump(dump_context, buf, len + 1);
 
-			len = snprintf(buf, sizeof(buf),
+			len = scnprintf(buf, sizeof(buf),
 				"  Bulk: tx_count=%d, tx_bytes=%llu, "
 				"rx_count=%d, rx_bytes=%llu",
 				service->stats.bulk_tx_count,
@@ -3486,7 +3486,7 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
 				service->stats.bulk_rx_bytes);
 			vchiq_dump(dump_context, buf, len + 1);
 
-			len = snprintf(buf, sizeof(buf),
+			len = scnprintf(buf, sizeof(buf),
 				"  %d quota stalls, %d slot stalls, "
 				"%d bulk stalls, %d aborted, %d errors",
 				service->stats.quota_stalls,
@@ -3562,9 +3562,9 @@ void vchiq_log_dump_mem(const char *label, u32 addr, const void *void_mem,
 
 		for (offset = 0; offset < 16; offset++) {
 			if (offset < num_bytes)
-				s += snprintf(s, 4, "%02x ", mem[offset]);
+				s += scnprintf(s, 4, "%02x ", mem[offset]);
 			else
-				s += snprintf(s, 4, "   ");
+				s += scnprintf(s, 4, "   ");
 		}
 
 		for (offset = 0; offset < 16; offset++) {
-- 
2.17.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
  2019-09-11 13:51 [PATCH] staging: vhciq_core: replace snprintf with scnprintf Rohit Sarkar
@ 2019-09-11 14:17 ` Stefan Wahren
  2019-09-11 14:25   ` Rohit Sarkar
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Wahren @ 2019-09-11 14:17 UTC (permalink / raw)
  To: Rohit Sarkar, Eric Anholt, Nicolas Saenz Julienne
  Cc: devel, Greg Kroah-Hartman, kernel-janitors, Tobias Büttner,
	linux-rpi-kernel, Dominic Braun

Hi Rohit,

On 11.09.19 15:51, Rohit Sarkar wrote:
> When the number of bytes to be printed exceeds the limit snprintf
> returns the number of bytes that would have been printed (if there was
> no truncation). This might cause issues, hence use scnprintf which
> returns the actual number of bytes printed to buffer always
>
> Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
thanks for your patch. Did you test your change on the Raspberry Pi?
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
  2019-09-11 14:17 ` Stefan Wahren
@ 2019-09-11 14:25   ` Rohit Sarkar
  2019-09-11 14:46     ` Dan Carpenter
  0 siblings, 1 reply; 7+ messages in thread
From: Rohit Sarkar @ 2019-09-11 14:25 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: devel, Greg Kroah-Hartman, kernel-janitors, Tobias Büttner,
	Eric Anholt, linux-rpi-kernel, Dominic Braun

On Wed, Sep 11, 2019 at 04:17:25PM +0200, Stefan Wahren wrote:
> Hi Rohit,
> 
> On 11.09.19 15:51, Rohit Sarkar wrote:
> > When the number of bytes to be printed exceeds the limit snprintf
> > returns the number of bytes that would have been printed (if there was
> > no truncation). This might cause issues, hence use scnprintf which
> > returns the actual number of bytes printed to buffer always
> >
> > Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> thanks for your patch. Did you test your change on the Raspberry Pi?

Hey Stefan,
No I haven't done so as I thought this is a generic change?
Will that be necessary? 
I am relatively new to kernel development
Thanks,
Rohit
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
  2019-09-11 14:25   ` Rohit Sarkar
@ 2019-09-11 14:46     ` Dan Carpenter
  2019-09-11 15:03       ` Rohit Sarkar
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-09-11 14:46 UTC (permalink / raw)
  To: Rohit Sarkar
  Cc: devel, Greg Kroah-Hartman, kernel-janitors, Tobias Büttner,
	Eric Anholt, Stefan Wahren, Dominic Braun, linux-rpi-kernel

On Wed, Sep 11, 2019 at 07:55:43PM +0530, Rohit Sarkar wrote:
> On Wed, Sep 11, 2019 at 04:17:25PM +0200, Stefan Wahren wrote:
> > Hi Rohit,
> > 
> > On 11.09.19 15:51, Rohit Sarkar wrote:
> > > When the number of bytes to be printed exceeds the limit snprintf
> > > returns the number of bytes that would have been printed (if there was
> > > no truncation). This might cause issues, hence use scnprintf which
> > > returns the actual number of bytes printed to buffer always
> > >
> > > Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> > thanks for your patch. Did you test your change on the Raspberry Pi?
> 
> Hey Stefan,
> No I haven't done so as I thought this is a generic change?
> Will that be necessary?

No.  It's not required.  The patch is easy to audit and clearly
harmless.

The question would be does it actually fix a bug?  I looked at it and
some of the strings are definitely a bit long.  The longest one I saw
was:
	"  Slots: %d available (%d data), %d recyclable, %d stalls (%d data)",
	123456789 123456789 123456789 123456789 123456789 123456789 123456789

If you get a lot of stalls, then that looks like it could lead to a
read overflow (an information leak).  Either way this does make the
code a bit easier to audit so it seems like a nice cleanup.  Next time
though, I really would prefer if you put this sort analysis in your
commit message so I can just glance over it.  (I'm lazy).

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
  2019-09-11 14:46     ` Dan Carpenter
@ 2019-09-11 15:03       ` Rohit Sarkar
  2019-09-11 17:24         ` Dan Carpenter
  0 siblings, 1 reply; 7+ messages in thread
From: Rohit Sarkar @ 2019-09-11 15:03 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: devel, Greg Kroah-Hartman, kernel-janitors, Tobias Büttner,
	Eric Anholt, Stefan Wahren, Dominic Braun, linux-rpi-kernel

On Wed, Sep 11, 2019 at 05:46:12PM +0300, Dan Carpenter wrote:
> On Wed, Sep 11, 2019 at 07:55:43PM +0530, Rohit Sarkar wrote:
> > On Wed, Sep 11, 2019 at 04:17:25PM +0200, Stefan Wahren wrote:
> > > Hi Rohit,
> > > 
> > > On 11.09.19 15:51, Rohit Sarkar wrote:
> > > > When the number of bytes to be printed exceeds the limit snprintf
> > > > returns the number of bytes that would have been printed (if there was
> > > > no truncation). This might cause issues, hence use scnprintf which
> > > > returns the actual number of bytes printed to buffer always
> > > >
> > > > Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> > > thanks for your patch. Did you test your change on the Raspberry Pi?
> > 
> > Hey Stefan,
> > No I haven't done so as I thought this is a generic change?
> > Will that be necessary?
> 
> No.  It's not required.  The patch is easy to audit and clearly
> harmless.
> 
> The question would be does it actually fix a bug?  I looked at it and
> some of the strings are definitely a bit long.  The longest one I saw
> was:
> 	"  Slots: %d available (%d data), %d recyclable, %d stalls (%d data)",
> 	123456789 123456789 123456789 123456789 123456789 123456789 123456789
> 
> If you get a lot of stalls, then that looks like it could lead to a
> read overflow (an information leak).  Either way this does make the
> code a bit easier to audit so it seems like a nice cleanup.  Next time
> though, I really would prefer if you put this sort analysis in your
> commit message so I can just glance over it.  (I'm lazy).
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> regards,
> dan carpenter

Hey Dan,
Thanks for reviewing this :)
I will make sure to add some analysis the next time I do a clean up like
this.

There are a lot of usages of "snprintf" throughout the staging
directory (315 to be exact)
Would it be worthwhile to find ones that may cause an information leak
and replace them with "scnprintf"?
Thanks,
Rohit
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
  2019-09-11 15:03       ` Rohit Sarkar
@ 2019-09-11 17:24         ` Dan Carpenter
  2019-09-11 17:38           ` Rohit Sarkar
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-09-11 17:24 UTC (permalink / raw)
  To: Rohit Sarkar
  Cc: devel, Greg Kroah-Hartman, kernel-janitors, Tobias Büttner,
	Eric Anholt, Stefan Wahren, Dominic Braun, linux-rpi-kernel

On Wed, Sep 11, 2019 at 08:33:00PM +0530, Rohit Sarkar wrote:
> There are a lot of usages of "snprintf" throughout the staging
> directory (315 to be exact)
> Would it be worthwhile to find ones that may cause an information leak
> and replace them with "scnprintf"?

A lot of times it's really easy to see that the uses are safe, so
snprintf() is fine in that case.  If it's not obviously safe then change
it.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: vhciq_core: replace snprintf with scnprintf
  2019-09-11 17:24         ` Dan Carpenter
@ 2019-09-11 17:38           ` Rohit Sarkar
  0 siblings, 0 replies; 7+ messages in thread
From: Rohit Sarkar @ 2019-09-11 17:38 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: devel, Greg Kroah-Hartman, kernel-janitors, Tobias Büttner,
	Eric Anholt, Stefan Wahren, Dominic Braun, linux-rpi-kernel

On Wed, Sep 11, 2019 at 08:24:22PM +0300, Dan Carpenter wrote:
> On Wed, Sep 11, 2019 at 08:33:00PM +0530, Rohit Sarkar wrote:
> > There are a lot of usages of "snprintf" throughout the staging
> > directory (315 to be exact)
> > Would it be worthwhile to find ones that may cause an information leak
> > and replace them with "scnprintf"?
> 
> A lot of times it's really easy to see that the uses are safe, so
> snprintf() is fine in that case.  If it's not obviously safe then change
> it.
> 
> regards,
> dan carpenter
> 

Sure, thanks a ton!
regards,
Rohit
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 13:51 [PATCH] staging: vhciq_core: replace snprintf with scnprintf Rohit Sarkar
2019-09-11 14:17 ` Stefan Wahren
2019-09-11 14:25   ` Rohit Sarkar
2019-09-11 14:46     ` Dan Carpenter
2019-09-11 15:03       ` Rohit Sarkar
2019-09-11 17:24         ` Dan Carpenter
2019-09-11 17:38           ` Rohit Sarkar

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org driverdev-devel@archiver.kernel.org
	public-inbox-index driverdev-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox