[PATCH 2.6] 2/3 Serio: possible race in handle_events

[PATCH 2.6] 2/3 Serio: possible race in handle_events

[Dmitry Torokhov]

The event handling in serio is racy because serio_handle_events does not
check whether serio in event list is still alive or has already been freed.
One possible solution is to check whether serio in question is still 
registered in serio_list and skip events referring to unknown serios.

Dmitry

diff -urN --exclude-from=/usr/src/exclude 2.6.0-test4/drivers/input/serio/serio.c linux-2.6.0-test4/drivers/input/serio/serio.c
--- 2.6.0-test4/drivers/input/serio/serio.c	2003-08-22 23:26:50.000000000 -0500
+++ linux-2.6.0-test4/drivers/input/serio/serio.c	2003-08-22 23:25:20.000000000 -0500
@@ -85,6 +85,16 @@
 static DECLARE_WAIT_QUEUE_HEAD(serio_wait);
 static DECLARE_COMPLETION(serio_exited);
 
+static int is_known_serio(struct serio *serio)
+{
+	struct serio *s;
+	
+	list_for_each_entry(s, &serio_list, node)
+		if (s == serio)
+			return 1;
+	return 0;
+}
+
 void serio_handle_events(void)
 {
 	struct list_head *node, *next;
@@ -93,17 +103,21 @@
 	list_for_each_safe(node, next, &serio_event_list) {
 		event = container_of(node, struct serio_event, node);	
 
+		down(&serio_sem);
+		if (!is_known_serio(event->serio))
+			goto event_done;
+		
 		switch (event->type) {
 			case SERIO_RESCAN :
-				down(&serio_sem);
 				if (event->serio->dev && event->serio->dev->disconnect)
 					event->serio->dev->disconnect(event->serio);
 				serio_find_dev(event->serio);
-				up(&serio_sem);
 				break;
 			default:
 				break;
 		}
+event_done:	
+		up(&serio_sem);
 		list_del_init(node);
 		kfree(event);
 	}

Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events

[Andrew Morton]

Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
> +static int is_known_serio(struct serio *serio)
>  +{
>  +	struct serio *s;
>  +	
>  +	list_for_each_entry(s, &serio_list, node)
>  +		if (s == serio)
>  +			return 1;
>  +	return 0;
>  +}

Could this just be

	return !list_empty(&serio->node);

?

Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events

[Andrew Morton]

Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
> On Saturday 23 August 2003 02:00 am, Andrew Morton wrote:
> > Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > > +static int is_known_serio(struct serio *serio)
> > >  +{
> > >  +	struct serio *s;
> > >  +
> > >  +	list_for_each_entry(s, &serio_list, node)
> > >  +		if (s == serio)
> > >  +			return 1;
> > >  +	return 0;
> > >  +}
> >
> > Could this just be
> >
> > 	return !list_empty(&serio->node);
> >
> > ?
> 
> The serio could be free()d, I dont think we want to call list_empty with 
> a dangling pointer. Or am I missing something?
> 

Well if we're playing around with a freed pointer then something is
seriously wrong.  Like, someone could have allocated a new one and got the
same address.

If event->serio can point at freed memory and there's any doubt over it
then we should be nulling out event->serio to indicate that.

Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events

[Dmitry Torokhov]

On Saturday 23 August 2003 02:19 am, Andrew Morton wrote:
> Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > On Saturday 23 August 2003 02:00 am, Andrew Morton wrote:
> > > Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > > > +static int is_known_serio(struct serio *serio)
> > > >  +{
> > > >  +	struct serio *s;
> > > >  +
> > > >  +	list_for_each_entry(s, &serio_list, node)
> > > >  +		if (s == serio)
> > > >  +			return 1;
> > > >  +	return 0;
> > > >  +}
> > >
> > > Could this just be
> > >
> > > 	return !list_empty(&serio->node);
> > >
> > > ?
> >
> > The serio could be free()d, I dont think we want to call list_empty with
> > a dangling pointer. Or am I missing something?
>
> Well if we're playing around with a freed pointer then something is
> seriously wrong.  Like, someone could have allocated a new one and got the
> same address.
>
> If event->serio can point at freed memory and there's any doubt over it
> then we should be nulling out event->serio to indicate that.

Right now we can't as events are queued in an event list and are processed by 
other thread; serio does not know that it's queued and even existence of the
event list is not known outside of serio.c module.

Do you think we should introduce allocate_serio/free_serio pair for dynamically 
allocated serios; free_serio would scan event_list and invalidate events that 
refer to freed serio?

Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events

[Andrew Morton]

Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
> Do you think we should introduce allocate_serio/free_serio pair for dynamically 
>  allocated serios; free_serio would scan event_list and invalidate events that 
>  refer to freed serio?

I don't understand the subsystem well enough to say.  But if we are
receiving events which refer to an already-freed serio then something is
very broken.  We should be draining all those events before allowing the
original object to be freed up.

Let's wait for Vojtech's comments.

Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events

[Zwane Mwaikambo]

On Sat, 23 Aug 2003, Andrew Morton wrote:

> Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> >
> > Do you think we should introduce allocate_serio/free_serio pair for dynamically 
> >  allocated serios; free_serio would scan event_list and invalidate events that 
> >  refer to freed serio?
> 
> I don't understand the subsystem well enough to say.  But if we are
> receiving events which refer to an already-freed serio then something is
> very broken.  We should be draining all those events before allowing the
> original object to be freed up.
> 
> Let's wait for Vojtech's comments.

This sounds somewhat like;

http://bugzilla.kernel.org/show_bug.cgi?id=973

Jul 25 04:54:24 lap kernel: slab error in cache_free_debugcheck(): cache 
`size-32': double free, or memory before object was overwritten
Jul 25 04:54:24 lap kernel: Call Trace:
Jul 25 04:54:24 lap kernel:  [<c014f130>] kfree+0xf0/0x310
Jul 25 04:54:24 lap kernel:  [<c02540fc>] psmouse_disconnect+0x2c/0x40
Jul 25 04:54:24 lap kernel:  [<c02540fc>] psmouse_disconnect+0x2c/0x40
Jul 25 04:54:24 lap kernel:  [<c025560d>] serio_handle_events+0xad/0xc0
Jul 25 04:54:24 lap kernel:  [<c0255620>] serio_thread+0x0/0x100
Jul 25 04:54:24 lap kernel:  [<c0255665>] serio_thread+0x45/0x100
Jul 25 04:54:24 lap kernel:  [<c010a126>] work_resched+0x5/0x16
Jul 25 04:54:24 lap kernel:  [<c0255620>] serio_thread+0x0/0x100
Jul 25 04:54:24 lap kernel:  [<c0255620>] serio_thread+0x0/0x100
Jul 25 04:54:24 lap kernel:  [<c01073b9>] kernel_thread_helper+0x5/0xc
Jul 25 04:54:24 lap kernel:
Jul 25 04:54:24 lap kernel: slab error in cache_free_debugcheck(): cache 
`size-32': double free, or memory after  object was overwritten

Jul 25 04:54:24 lap kernel: Call Trace:
Jul 25 04:54:24 lap kernel:  [<c014f15e>] kfree+0x11e/0x310
Jul 25 04:54:24 lap kernel:  [<c02540fc>] psmouse_disconnect+0x2c/0x40
Jul 25 04:54:24 lap kernel:  [<c02540fc>] psmouse_disconnect+0x2c/0x40
Jul 25 04:54:24 lap kernel:  [<c025560d>] serio_handle_events+0xad/0xc0
Jul 25 04:54:24 lap kernel:  [<c0255620>] serio_thread+0x0/0x100
Jul 25 04:54:24 lap kernel:  [<c0255665>] serio_thread+0x45/0x100
Jul 25 04:54:24 lap kernel:  [<c010a126>] work_resched+0x5/0x16
Jul 25 04:54:24 lap kernel:  [<c0255620>] serio_thread+0x0/0x100
Jul 25 04:54:24 lap kernel:  [<c0255620>] serio_thread+0x0/0x100
Jul 25 04:54:24 lap kernel:  [<c01073b9>] kernel_thread_helper+0x5/0xc
Jul 25 04:54:24 lap kernel:

Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events

[Dmitry Torokhov]

On Saturday 23 August 2003 02:34 am, Andrew Morton wrote:
> Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > Do you think we should introduce allocate_serio/free_serio pair for
> > dynamically allocated serios; free_serio would scan event_list and
> > invalidate events that refer to freed serio?
>
> I don't understand the subsystem well enough to say.  But if we are
> receiving events which refer to an already-freed serio then something is
> very broken.  We should be draining all those events before allowing the
> original object to be freed up.
>
> Let's wait for Vojtech's comments.

I think the patch below should work. We invalidate serio events at the same
time serio itself is unregistered.

The patch is on top of the 3 previous ones + synaptics, but if needed I can 
rediff against vanilla kernel.


diff -urN --exclude-from=/usr/src/exclude 2.6.0-test4/drivers/input/serio/serio.c linux-2.6.0-test4/drivers/input/serio/serio.c
--- 2.6.0-test4/drivers/input/serio/serio.c	2003-08-23 00:38:10.000000000 -0500
+++ linux-2.6.0-test4/drivers/input/serio/serio.c	2003-08-23 16:28:28.000000000 -0500
@@ -89,14 +89,13 @@
 static DECLARE_WAIT_QUEUE_HEAD(serio_wait);
 static DECLARE_COMPLETION(serio_exited);
 
-static int is_known_serio(struct serio *serio)
+static void serio_invalidate_pending_events(struct serio *serio)
 {
-	struct serio *s;
+	struct serio_event *event;
 	
-	list_for_each_entry(s, &serio_list, node)
-		if (s == serio)
-			return 1;
-	return 0;
+	list_for_each_entry(event, &serio_event_list, node)
+		if (event->serio == serio)
+			event->serio = NULL;
 }
 
 void serio_handle_events(void)
@@ -108,7 +107,7 @@
 		event = container_of(node, struct serio_event, node);	
 
 		down(&serio_sem);
-		if (!is_known_serio(event->serio))
+		if (event->serio == NULL)
 			goto event_done;
 		
 		switch (event->type) {
@@ -213,6 +212,7 @@
 void serio_unregister_port(struct serio *serio)
 {
 	down(&serio_sem);
+	serio_invalidate_pending_events(serio);
 	list_del_init(&serio->node);
 	if (serio->dev && serio->dev->disconnect)
 		serio->dev->disconnect(serio);
@@ -226,6 +226,7 @@
  */
 void serio_unregister_slave_port(struct serio *serio)
 {
+	serio_invalidate_pending_events(serio);
 	list_del_init(&serio->node);
 	if (serio->dev && serio->dev->disconnect)
 		serio->dev->disconnect(serio);