[patch] virtio_console: use snprintf() for safety

public inbox for kernel-janitors@vger.kernel.org
 help / color / mirror / Atom feed

* [patch] virtio_console: use snprintf() for safety
@ 2015-05-08  6:19 Dan Carpenter
  2015-05-08  6:43 ` Amit Shah
  0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2015-05-08  6:19 UTC (permalink / raw)
  To: kernel-janitors

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@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,

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

* Re: [patch] virtio_console: use snprintf() for safety
  2015-05-08  6:19 [patch] virtio_console: use snprintf() for safety Dan Carpenter
@ 2015-05-08  6:43 ` Amit Shah
  2015-05-08  9:16   ` [patch v2] virtio_console: silence a static checker warning Dan Carpenter
  0 siblings, 1 reply; 9+ messages in thread
From: Amit Shah @ 2015-05-08  6:43 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Greg Kroah-Hartman, kernel-janitors, Arnd Bergmann,
	virtualization

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

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

* [patch v2] virtio_console: silence a static checker warning
  2015-05-08  6:43 ` Amit Shah
@ 2015-05-08  9:16   ` Dan Carpenter
  2015-05-08  9:30     ` walter harms
                       ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Dan Carpenter @ 2015-05-08  9:16 UTC (permalink / raw)
  To: Amit Shah
  Cc: Greg Kroah-Hartman, kernel-janitors, Arnd Bergmann,
	virtualization

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@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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [patch v2] virtio_console: silence a static checker warning
  2015-05-08  9:16   ` [patch v2] virtio_console: silence a static checker warning Dan Carpenter
@ 2015-05-08  9:30     ` walter harms
  2015-05-08  9:41     ` Amit Shah
                       ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: walter harms @ 2015-05-08  9:30 UTC (permalink / raw)
  To: kernel-janitors



Am 08.05.2015 11:16, schrieb Dan Carpenter:
> 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@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);


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

>  		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@vger.kernel.org
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

* Re: [patch v2] virtio_console: silence a static checker warning
  2015-05-08  9:16   ` [patch v2] virtio_console: silence a static checker warning Dan Carpenter
  2015-05-08  9:30     ` walter harms
@ 2015-05-08  9:41     ` Amit Shah
  2015-05-08  9:47     ` Dan Carpenter
                       ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Amit Shah @ 2015-05-08  9:41 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Greg Kroah-Hartman, kernel-janitors, Arnd Bergmann,
	virtualization

On (Fri) 08 May 2015 [12:16:25], Dan Carpenter wrote:
> 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 <amit.shah@redhat.com>

Thanks!


		Amit

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

* Re: [patch v2] virtio_console: silence a static checker warning
  2015-05-08  9:16   ` [patch v2] virtio_console: silence a static checker warning Dan Carpenter
  2015-05-08  9:30     ` walter harms
  2015-05-08  9:41     ` Amit Shah
@ 2015-05-08  9:47     ` Dan Carpenter
  2015-05-08  9:56     ` Amit Shah
  2015-05-08 11:13     ` walter harms
  4 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2015-05-08  9:47 UTC (permalink / raw)
  To: kernel-janitors

On Fri, May 08, 2015 at 11:30:09AM +0200, walter harms wrote:
> 
> 
> Am 08.05.2015 11:16, schrieb Dan Carpenter:
> > 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@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);
> 
> 
> 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.

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

regards,
dan carpenter


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

* Re: [patch v2] virtio_console: silence a static checker warning
  2015-05-08  9:16   ` [patch v2] virtio_console: silence a static checker warning Dan Carpenter
                       ` (2 preceding siblings ...)
  2015-05-08  9:47     ` Dan Carpenter
@ 2015-05-08  9:56     ` Amit Shah
  2015-05-08 11:13     ` walter harms
  4 siblings, 0 replies; 9+ messages in thread
From: Amit Shah @ 2015-05-08  9:56 UTC (permalink / raw)
  To: kernel-janitors

On (Fri) 08 May 2015 [11:30:09], walter harms wrote:
> 
> 
> Am 08.05.2015 11:16, schrieb Dan Carpenter:
> > 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@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);
> 
> 
> 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.

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

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

* Re: [patch v2] virtio_console: silence a static checker warning
  2015-05-08  9:16   ` [patch v2] virtio_console: silence a static checker warning Dan Carpenter
                       ` (3 preceding siblings ...)
  2015-05-08  9:56     ` Amit Shah
@ 2015-05-08 11:13     ` walter harms
  2015-05-08 12:18       ` Dan Carpenter
  4 siblings, 1 reply; 9+ messages in thread
From: walter harms @ 2015-05-08 11:13 UTC (permalink / raw)
  To: kernel-janitors



Am 08.05.2015 11:56, schrieb Amit Shah:
> On (Fri) 08 May 2015 [11:30:09], walter harms wrote:
>>
>>
>> Am 08.05.2015 11:16, schrieb Dan Carpenter:
>>> 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@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);
>>
>>
>> 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.
> 
> 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


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

* Re: [patch v2] virtio_console: silence a static checker warning
  2015-05-08 11:13     ` walter harms
@ 2015-05-08 12:18       ` Dan Carpenter
  0 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2015-05-08 12:18 UTC (permalink / raw)
  To: walter harms
  Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, Arnd Bergmann,
	virtualization

On Fri, May 08, 2015 at 01:13:22PM +0200, walter harms wrote:
> 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


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

end of thread, other threads:[~2015-05-08 12:18 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-08  6:19 [patch] virtio_console: use snprintf() for safety Dan Carpenter
2015-05-08  6:43 ` Amit Shah
2015-05-08  9:16   ` [patch v2] virtio_console: silence a static checker warning Dan Carpenter
2015-05-08  9:30     ` walter harms
2015-05-08  9:41     ` Amit Shah
2015-05-08  9:47     ` Dan Carpenter
2015-05-08  9:56     ` Amit Shah
2015-05-08 11:13     ` walter harms
2015-05-08 12:18       ` Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox