Linux Virtualization - May 2015 - [patch] 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 <dan.carpenter at oracle.com>

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 50754d20..8283989 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -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,

On (Fri) 08 May 2015 [09:19:02], Dan Carpenter wrote:
> 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..
> 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

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 <dan.carpenter at oracle.com>
---
v2: the overflow is not possible so just leave the buffer size alone and
    silence the warning with snprintf().

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 50754d20..8283989 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -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 majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html