virtio_console: use snprintf() for safety

PATCH In review Dan Carpenter 3 participants 13 messages May 8, 2015

Subsystems (4)

Kernel janitors Char and misc drivers virtio core virtio console driver

lore

Share & tools

lore

Dan Carpenter

author

2015-05-08 06:19 UTC #0 lore raw

PATCH 1/1 virtio_console: use snprintf() for safety

My static checker complains that this sprintf() can overflow.

vdev->index is selected by ida_simple_get() in register_virtio_device() so my reading of the code is that this overflow is theoretically possible. The max value of "id" is configurable and I'm not sure what typical values are.

Anyway, it's simple enough to make the buffer larger and I changed it to snprintf() as well.

Signed-off-by Dan Carpenter <[email protected]>

drivers/char/virtio_console.c +3 -3

index 50754d20..8283989 100644
@@ -1389,7 +1389,7 @@ static void send_sigio_to_port(struct port *port)
 
 static int add_port(struct ports_device *portdev, u32 id)
 {
-	char debugfs_name[16];
+	char debugfs_name[28];
 	struct port *port;
 	struct port_buffer *buf;
 	dev_t devt;

@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);
 		port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
 							 pdrvdata.debugfs_dir,
 							 port,

Amit Shah

2015-05-08 06:31 UTC #1 lore raw

Quoted 6 lines · Dan Carpenter

My static checker complains that this sprintf() can overflow.
vdev->index is selected by ida_simple_get() in register_virtio_device() so my reading of the code is that this overflow is theoretically possible. The max value of "id" is configurable and I'm not sure what typical values are.

vdev->index is per-device, and starts with 0 for the first attached virtio-serial-pci device. So to overflow, a lot of devices have to be attached, which isn't possible with current qemu. 16 bytes was already overkill..

Quoted 2 lines

Anyway, it's simple enough to make the buffer larger and I changed it to snprintf() as well.

Any reason to choose 28? I think 16 is enough.

The snprintf change is fine, though.

Amit

Dan Carpenter

author

2015-05-08 09:16 UTC #2 lore raw

PATCH v2 1/1 virtio_console: silence a static checker warning

My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().

Signed-off-by Dan Carpenter <[email protected]>

---
v2: the overflow is not possible so just leave the buffer size alone and
silence the warning with snprintf().

drivers/char/virtio_console.c +2 -2

index 50754d20..8283989 100644
@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);
 		port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
 							 pdrvdata.debugfs_dir,
 							 port,

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Amit Shah

Reviewed-by on v2 1/1

2015-05-08 09:29 UTC #3 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

Quoted 2 lines · Dan Carpenter

My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().

Reviewed-by Amit Shah <[email protected]>

Thanks!

Amit

walter harms

2015-05-08 09:30 UTC #4 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

In reply to Dan Carpenter

Am 08.05.2015 11:16, schrieb Dan Carpenter:

Quoted 13 lines

My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().
Signed-off-by Dan Carpenter <[email protected]>
---
v2: the overflow is not possible so just leave the buffer size alone and
silence the warning with snprintf().
drivers/char/virtio_console.c +0 -0
index 50754d20..8283989 100644

drivers/char/virtio_console.c static int add_port(struct ports_device *portdev, u32 id) · line 1496

Quoted 7 lines

drivers/char/virtio_console.c +2 -2

@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);

would it help to use %03u (or so) to make it more obvious ?

Note: i prefer a leading 0 in my programms to make it more easy to work with grep and friends. you may thing otherwise.

re,
wh

Quoted 12 lines

drivers/char/virtio_console.c +0 -1
 		port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
 							 pdrvdata.debugfs_dir,
 							 port,
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

walter harms

2015-05-08 09:30 UTC #7 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

In reply to Dan Carpenter

Am 08.05.2015 11:16, schrieb Dan Carpenter:

Quoted 13 lines

My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().
Signed-off-by Dan Carpenter <[email protected]>
---
v2: the overflow is not possible so just leave the buffer size alone and
silence the warning with snprintf().
drivers/char/virtio_console.c +0 -0
index 50754d20..8283989 100644

drivers/char/virtio_console.c static int add_port(struct ports_device *portdev, u32 id) · line 1496

Quoted 7 lines

drivers/char/virtio_console.c +2 -2

@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);

would it help to use %03u (or so) to make it more obvious ?

Note: i prefer a leading 0 in my programms to make it more easy to work with grep and friends. you may thing otherwise.

re,
wh

Quoted 12 lines

drivers/char/virtio_console.c +0 -1
 		port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
 							 pdrvdata.debugfs_dir,
 							 port,
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2015-05-08 09:47 UTC #5 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

In reply to walter harms

Quoted 27 lines · walter harms

Am 08.05.2015 11:16, schrieb Dan Carpenter:
Quoted 13 lines
My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().
Signed-off-by Dan Carpenter <[email protected]>
---
v2: the overflow is not possible so just leave the buffer size alone and
silence the warning with snprintf().
drivers/char/virtio_console.c +0 -0
index 50754d20..8283989 100644
drivers/char/virtio_console.c static int add_port(struct ports_device *portdev, u32 id) · line 1496
Quoted 7 lines
drivers/char/virtio_console.c +2 -2
@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);
would it help to use %03u (or so) to make it more obvious ?

Quoted 2 lines

Note: i prefer a leading 0 in my programms to make it more easy to work with grep and friends. you may thing otherwise.

That's an user API change so it's probably a bad idea.

regards,
dan carpenter

Dan Carpenter

author

2015-05-08 09:47 UTC #8 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

In reply to Dan Carpenter

Quoted 27 lines · walter harms

Am 08.05.2015 11:16, schrieb Dan Carpenter:
Quoted 13 lines
My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().
Signed-off-by Dan Carpenter <[email protected]>
---
v2: the overflow is not possible so just leave the buffer size alone and
silence the warning with snprintf().
drivers/char/virtio_console.c +0 -0
index 50754d20..8283989 100644
drivers/char/virtio_console.c static int add_port(struct ports_device *portdev, u32 id) · line 1496
Quoted 7 lines
drivers/char/virtio_console.c +2 -2
@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);
would it help to use %03u (or so) to make it more obvious ?

Quoted 2 lines

Note: i prefer a leading 0 in my programms to make it more easy to work with grep and friends. you may thing otherwise.

That's an user API change so it's probably a bad idea.

regards,
dan carpenter

Amit Shah

2015-05-08 09:56 UTC #6 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

In reply to walter harms

Quoted 27 lines · walter harms

Am 08.05.2015 11:16, schrieb Dan Carpenter:
Quoted 13 lines
My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().
Signed-off-by Dan Carpenter <[email protected]>
---
v2: the overflow is not possible so just leave the buffer size alone and
silence the warning with snprintf().
drivers/char/virtio_console.c +0 -0
index 50754d20..8283989 100644
drivers/char/virtio_console.c static int add_port(struct ports_device *portdev, u32 id) · line 1496
Quoted 7 lines
drivers/char/virtio_console.c +2 -2
@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);
would it help to use %03u (or so) to make it more obvious ?

Quoted 2 lines

Note: i prefer a leading 0 in my programms to make it more easy to work with grep and friends. you may thing otherwise.

Well we've been exposing names like /dev/vport0p0, /dev/vport2p15, etc., and there might be scripts relying on such names, so that's one argument against it.

However we do have pretty names that map to these ports via udev rules, but not sure if we should change the name just to prepend 0s.

Amit

Amit Shah

2015-05-08 09:56 UTC #9 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

In reply to Dan Carpenter

Quoted 27 lines · walter harms

Am 08.05.2015 11:16, schrieb Dan Carpenter:
Quoted 13 lines
My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().
Signed-off-by Dan Carpenter <[email protected]>
---
v2: the overflow is not possible so just leave the buffer size alone and
silence the warning with snprintf().
drivers/char/virtio_console.c +0 -0
index 50754d20..8283989 100644
drivers/char/virtio_console.c static int add_port(struct ports_device *portdev, u32 id) · line 1496
Quoted 7 lines
drivers/char/virtio_console.c +2 -2
@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);
would it help to use %03u (or so) to make it more obvious ?

Quoted 2 lines

Note: i prefer a leading 0 in my programms to make it more easy to work with grep and friends. you may thing otherwise.

Well we've been exposing names like /dev/vport0p0, /dev/vport2p15, etc., and there might be scripts relying on such names, so that's one argument against it.

However we do have pretty names that map to these ports via udev rules, but not sure if we should change the name just to prepend 0s.

Amit

walter harms

2015-05-08 11:13 UTC #10 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

Am 08.05.2015 11:56, schrieb Amit Shah:

Quoted 31 lines · walter harms

Quoted 27 lines
Am 08.05.2015 11:16, schrieb Dan Carpenter:
Quoted 13 lines
My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().
Signed-off-by Dan Carpenter <[email protected]>
---
v2: the overflow is not possible so just leave the buffer size alone and
silence the warning with snprintf().
drivers/char/virtio_console.c +0 -0
index 50754d20..8283989 100644
drivers/char/virtio_console.c static int add_port(struct ports_device *portdev, u32 id) · line 1496
Quoted 7 lines
drivers/char/virtio_console.c +2 -2
@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);
would it help to use %03u (or so) to make it more obvious ?
Quoted 2 lines
Note: i prefer a leading 0 in my programms to make it more easy to work with grep and friends. you may thing otherwise.

Quoted 9 lines

Well we've been exposing names like /dev/vport0p0, /dev/vport2p15, etc., and there might be scripts relying on such names, so that's one argument against it.
However we do have pretty names that map to these ports via udev rules, but not sure if we should change the name just to prepend 0s.
Amit

The basic idea was to limit the space, having leading zeros is my idea because i found this more convenient in the past. Using something like "%3u" will give static checkers a chance to detect the required max. space.

re,
wh

walter harms

2015-05-08 11:13 UTC #11 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

In reply to Dan Carpenter

Am 08.05.2015 11:56, schrieb Amit Shah:

Quoted 31 lines · walter harms

Quoted 27 lines
Am 08.05.2015 11:16, schrieb Dan Carpenter:
Quoted 13 lines
My static checker complains that this sprintf() can overflow but really it can't. Just silence the warning by using snprintf().
Signed-off-by Dan Carpenter <[email protected]>
---
v2: the overflow is not possible so just leave the buffer size alone and
silence the warning with snprintf().
drivers/char/virtio_console.c +0 -0
index 50754d20..8283989 100644
drivers/char/virtio_console.c static int add_port(struct ports_device *portdev, u32 id) · line 1496
Quoted 7 lines
drivers/char/virtio_console.c +2 -2
@@ -1492,8 +1492,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 * Finally, create the debugfs file that we can use to
 		 * inspect a port's state at any time
 		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->vdev->index, id);
+		snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
+			 port->portdev->vdev->index, id);
would it help to use %03u (or so) to make it more obvious ?
Quoted 2 lines
Note: i prefer a leading 0 in my programms to make it more easy to work with grep and friends. you may thing otherwise.

Quoted 9 lines

Well we've been exposing names like /dev/vport0p0, /dev/vport2p15, etc., and there might be scripts relying on such names, so that's one argument against it.
However we do have pretty names that map to these ports via udev rules, but not sure if we should change the name just to prepend 0s.
Amit

re,
wh

Dan Carpenter

author

2015-05-08 12:18 UTC #12 lore raw

Re: [patch v2] virtio_console: silence a static checker warning

Quoted 3 lines · walter harms

The basic idea was to limit the space, having leading zeros is my idea because i found this more convenient in the past. Using something like "%3u" will give static checkers a chance to detect the required max. space.

How are you calculating the %3?

The max for "id" is currently 31. To be honest, I'm not certain the max for ->index. It's something in qemu but I'm not sure what. Who is going to keep it updated?

The %3 is sort of meaningless. The lower levels of the code just ignore it don't they?

regards,
dan carpenter