[PATCH] tty races

[Jason Baron]

There are a couple of tty race conditions, which lead to inconsistent tty 
reference counting and tty layer oopses.

The first is a tty_open vs. tty_close race in drivers/char/tty.io.c. 
Basically, from the time that the tty->count is deemed to be 1 and that we 
are going to free it to the time that TTY_CLOSING bit is set, needs to be 
atomic with respect to the manipulation of tty->count in init_dev(). This 
atomicity was previously guarded by the BKL. However, this is no longer 
true with the addition of a down() call in the middle of the 
release_dev()'s atomic path. So either the down() needs to be moved 
outside the atomic patch or dropped. I would vote for simply dropping it 
as i don't see why it is necessary.

The second race is tty_open vs. tty_open. This race I've seen when the 
virtual console is the tty driver. In con_open(),  vc_allocate() is called 
if the tty->count is 1. However, this check of the tty->count is not 
guarded by the 'tty_sem'. Thus, it is possible for con_open(), to never 
see the tty->count as 1, and thus never call vc_allocate(). This leads to 
a NULL filp->private_data, and an oops.

The test case below reproduces these problems, and the patch fixes it. The 
test case uses /dev/tty9, which is generally restricted to root for 
open(). It may be able to exploit these races using pseudo terminals, 
although i wasn't able to. A previous report of this issue, with an oops 
trace was: http://www.ussg.iu.edu/hypermail/linux/kernel/0503.2/0017.html

thanks,

-Jason


--- linux/drivers/char/tty_io.c.bak
+++ linux/drivers/char/tty_io.c
@@ -1596,14 +1596,9 @@ static void release_dev(struct file * fi
 	 * each iteration we avoid any problems.
 	 */
 	while (1) {
-		/* Guard against races with tty->count changes elsewhere and
-		   opens on /dev/tty */
-		   
-		down(&tty_sem);
 		tty_closing = tty->count <= 1;
 		o_tty_closing = o_tty &&
 			(o_tty->count <= (pty_master ? 1 : 0));
-		up(&tty_sem);
 		do_sleep = 0;
 
 		if (tty_closing) {
@@ -1640,7 +1635,6 @@ static void release_dev(struct file * fi
 	 * block, so it's safe to proceed with closing.
 	 */
 	 
-	down(&tty_sem);
 	if (pty_master) {
 		if (--o_tty->count < 0) {
 			printk(KERN_WARNING "release_dev: bad pty slave count "
@@ -1654,7 +1648,6 @@ static void release_dev(struct file * fi
 		       tty->count, tty_name(tty, buf));
 		tty->count = 0;
 	}
-	up(&tty_sem);
 	
 	/*
 	 * We've decremented tty->count, so we need to remove this file
@@ -1844,9 +1837,10 @@ retry_open:
 	}
 got_driver:
 	retval = init_dev(driver, index, &tty);
-	up(&tty_sem);
-	if (retval)
+	if (retval) {
+		up(&tty_sem);
 		return retval;
+	}
 
 	filp->private_data = tty;
 	file_move(filp, &tty->tty_files);
@@ -1863,6 +1857,7 @@ got_driver:
 		else
 			retval = -ENODEV;
 	}
+	up(&tty_sem);
 	filp->f_flags = saved_flags;
 
 	if (!retval && test_bit(TTY_EXCLUSIVE, &tty->flags) && !capable(CAP_SYS_ADMIN))



#include <sys/types.h>
#include <sys/stat.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <time.h>
#include <pthread.h>
#include <linux/fb.h>
#include <linux/vt.h>
#include <linux/kd.h>

#define NTHREADS 300

void *thread_function();
int open_fail_num;
int open_success;

int
main(int argc, char *argv[])
{
  	int i, j;
  	pthread_t thread_id[NTHREADS];

  	for(;;) {
      		for(i=0; i < NTHREADS; i++) {
	  		pthread_create(&thread_id[i], NULL, &thread_function, NULL);
		}
      		for(j=0; j < NTHREADS; j++) {
	  		pthread_join(thread_id[j], NULL); 
		}
      		printf("open failures: %i\n", open_fail_num);
      		printf("open success: %i\n", open_success);
    	}
}

void *thread_function()
{
  	int fd;
  	time_t t;
  	int val;
  	int ret;

  	fd = open("/dev/tty9", O_RDWR);

  	val = 0;
  	//call an ioctl
	ret = ioctl(fd, KDGETMODE, &val);
  	if (ret != 0) {
		perror("ioctl error\n");
  	}

  	if (fd < 0) {
      		open_fail_num++;
    	} else {
      		open_success++;
    	}
  	/* just waste some random time */
  	t = (time((time_t *)0) &31L) << 6;
  	while (t-- > 0)
    		(void)time((time_t *)0);
  	close(fd);	
}




-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/