2/3 Serio: possible race in handle_events

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
@@ -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);
 	}

Could this just be

	return !list_empty(&serio->node);

?

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?

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:

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
@@ -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);