[RFC][PATCH 2.6.17-rc6] input/mouse/sermouse: fix memleak and potential buffer overflow

[RFC][PATCH 2.6.17-rc6] input/mouse/sermouse: fix memleak and potential buffer overflow

[Wouter Paesen]

While strolling trough the sermouse driver for some example code, I
noticed 2 strange things happening there :

* In the sermouse_connect function an input device structure is
  allocated (input_allocate_device), which is not deallocated 
  in the sermouse_disconnect function.  
  
  If I understand this correctly someone repeatedly connecting and 
  disconnecting the mouse would leak input_dev structures.

* In the sermouse_connect function the phys member of the sermouse 
  structure (32 characters) is initialised with :

     sprintf(sermouse->phys, "%s/input0", serio->phys);

  Because serio->phys is also a 32 character field the sprintf could
  result in 39 characters being written to the sermouse->phys.

If my understanding of both these concepts is correct, this is a patch
to fix the problems.

Signed-off-by: Wouter Paesen <wouter@kangaroot.net>

--- a/drivers/input/mouse/sermouse.c	2006-06-15 08:47:47.000000000 +0200
+++ b/drivers/input/mouse/sermouse.c	2006-06-15 08:52:13.000000000 +0200
@@ -53,7 +53,7 @@
 	unsigned char count;
 	unsigned char type;
 	unsigned long last;
-	char phys[32];
+	char phys[39];
 };
 
 /*
@@ -233,6 +233,7 @@
 	serio_close(serio);
 	serio_set_drvdata(serio, NULL);
 	input_unregister_device(sermouse->dev);
+	input_free_device(sermouse->dev);
 	kfree(sermouse);
 }

Re: [RFC][PATCH 2.6.17-rc6] input/mouse/sermouse: fix memleak and potential buffer overflow

[Dmitry Torokhov]

On Thursday 15 June 2006 06:47, Wouter Paesen wrote:
> While strolling trough the sermouse driver for some example code, I
> noticed 2 strange things happening there :
> 
> * In the sermouse_connect function an input device structure is
>   allocated (input_allocate_device), which is not deallocated 
>   in the sermouse_disconnect function.  
>   
>   If I understand this correctly someone repeatedly connecting and 
>   disconnecting the mouse would leak input_dev structures.
>

No, input_free_device() should not be called after input_register_device()
returns successfully because input_dev will be freed automatically once
last reference to it is dropped.

> * In the sermouse_connect function the phys member of the sermouse 
>   structure (32 characters) is initialised with :
> 
>      sprintf(sermouse->phys, "%s/input0", serio->phys);
> 
>   Because serio->phys is also a 32 character field the sprintf could
>   result in 39 characters being written to the sermouse->phys.
>

Right, we need to change it to use snprintf.

-- 
Dmitry

Re: [RFC][PATCH 2.6.17-rc6] input/mouse/sermouse: fix memleak and potential buffer overflow

[Wouter Paesen]

On Sun, Jun 18, 2006 at 12:24:31AM -0400, Dmitry Torokhov wrote:
> >   Because serio->phys is also a 32 character field the sprintf could
> >   result in 39 characters being written to the sermouse->phys.
> 
> Right, we need to change it to use snprintf.

Thanks, this patch will do just that. 
Still, keeping the array 39 characters long will prevent truncation of the string.

Signed-off-by: Wouter Paesen <wouter@kangaroot.net>

--- linux-2.6.17-rc6.orig/drivers/input/mouse/sermouse.c 2006-06-20 08:31:12.000000000 +0200
+++ linux-2.6.17-rc6/drivers/input/mouse/sermouse.c 2006-06-20 08:31:41.000000000 +0200
@@ -53,7 +53,7 @@ struct sermouse {
 	unsigned char count;
 	unsigned char type;
 	unsigned long last;
-	char phys[32];
+	char phys[39];
 };
 
 /*
@@ -254,7 +254,7 @@ static int sermouse_connect(struct serio
 		goto fail;
 
 	sermouse->dev = input_dev;
-	sprintf(sermouse->phys, "%s/input0", serio->phys);
+	snprintf(sermouse->phys, 39, "%s/input0", serio->phys);
 	sermouse->type = serio->id.proto;
 
 	input_dev->name = sermouse_protocols[sermouse->type];

Re: [RFC][PATCH 2.6.17-rc6] input/mouse/sermouse: fix memleak and potential buffer overflow

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

Wouter Paesen wrote:
>On Sun, Jun 18, 2006 at 12:24:31AM -0400, Dmitry Torokhov wrote:
>> >   Because serio->phys is also a 32 character field the sprintf could
>> >   result in 39 characters being written to the sermouse->phys.
>>
>> Right, we need to change it to use snprintf.
>
>Thanks, this patch will do just that.
>Still, keeping the array 39 characters long will prevent truncation of the
> string.
>
>Signed-off-by: Wouter Paesen <wouter@kangaroot.net>
>
>--- linux-2.6.17-rc6.orig/drivers/input/mouse/sermouse.c 2006-06-20
> 08:31:12.000000000 +0200 +++
> linux-2.6.17-rc6/drivers/input/mouse/sermouse.c 2006-06-20
> 08:31:41.000000000 +0200 @@ -53,7 +53,7 @@ struct sermouse {
> 	unsigned char count;
> 	unsigned char type;
> 	unsigned long last;
>-	char phys[32];
>+	char phys[39];
> };
>
> /*
>@@ -254,7 +254,7 @@ static int sermouse_connect(struct serio
> 		goto fail;
>
> 	sermouse->dev = input_dev;
>-	sprintf(sermouse->phys, "%s/input0", serio->phys);
>+	snprintf(sermouse->phys, 39, "%s/input0", serio->phys);

This adds a magic number here. I suggest using sizeof(sermouse->phys) instead.

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]