[PATCH] race condition with drivers/char/vt.c (bug in vt_ioctl.c)

[PATCH] race condition with drivers/char/vt.c (bug in vt_ioctl.c)

[Steven Rostedt]

While debugging Ingo's RT patch, I came accross this race condition.
The mainline seems to be susceptible to this bug, although it may be 1
in a 1,000,000 to happen. But those are the nastiest races.

With debugging information in the RT patch, I was able to reproduce this
race several times. Enough to be able to debug it. 

The race is with the tty->driver_data, tty->count and vt.c

Here's the scoop:

Process P1 opens a tty:
  tty_open 
    --> init_dev
          sets tty->count to 1

P1 does what it needs to, and closes the tty.
  tty_release  (now showing locks)
    (grabs BKL)
    --> release_dev
        --> tty->driver->close ==> con_close (vt.c)
            (down tty_sem)
            (aquire console_sem)
            tty->driver_data = NULL

Now process P2 opens the console:
   tty_open
      (block on tty_sem)

back to P1
         (release console_sem)
         (up tty_sem)

back to P2
       (down tty_sem)
       --> init_dev
           tty->count++ (tty->count now == 2)
           (up tty_sem)
           --> tty->driver->open ==> con_open (vt.c)
              (aquire console_sem)
               if (tty->count == 1) (which it does not)
                    allocate tty->driver_data
                          (which doesn't happen)
              (release console_sem)
  
     And P2 goes happily along with driver_data == NULL.

Now in something like vt_ioctl (which I first saw the bug)

int vt_ioctl(struct tty_struct *tty, struct file * file,
	     unsigned int cmd, unsigned long arg)
{
	struct vc_data *vc = (struct vc_data *)tty->driver_data;
	struct console_font_op op;	/* used in multiple places here */
	struct kbd_struct * kbd;
	unsigned int console;
	unsigned char ucval;
	void __user *up = (void __user *)arg;
	int i, perm;
	
	console = vc->vc_num;

Where here vc->vc_num could very well be (0)->vc_num.

I googled a little and found where this may have already happened in the
main line kernel:

http://seclists.org/lists/linux-kernel/2005/Aug/1603.html

So here's my proposal: 

  Instead of checking for tty->count == 1 in con_open, which we see is
not reliable.  Just check for tty->driver_data == NULL.

This should work since it should always be NULL when we need to assign
it.  If we switch the events of the race, so that the init_dev went
first, the driver_data would not be NULL and would not need to be
allocated, because after init_dev tty->count would be greater than 1
(this is assuming the case that it is already allocated) and the
con_close would not deallocate it.  The tty_sem and console_sem and
order of events protect the tty->driver_data but not the tty->count.

Without the patch, I was able to get the system to BUG on bootup every
other time.  With the patch applied, I was able to bootup 6 out of 6
times without a single crash. 

-- Steve

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

--- linux-2.6.13-rc6-git10/drivers/char/vt.c.orig	2005-08-19 22:51:25.000000000 -0400
+++ linux-2.6.13-rc6-git10/drivers/char/vt.c	2005-08-19 22:52:22.000000000 -0400
@@ -2433,7 +2433,7 @@ static int con_open(struct tty_struct *t
 	int ret = 0;
 
 	acquire_console_sem();
-	if (tty->count == 1) {
+	if (tty->driver_data == NULL) {
 		ret = vc_allocate(currcons);
 		if (ret == 0) {
 			struct vc_data *vc = vc_cons[currcons].d;

Re: [PATCH] race condition with drivers/char/vt.c (bug in vt_ioctl.c)

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> I googled a little and found where this may have already happened in 
> the main line kernel:
> 
> http://seclists.org/lists/linux-kernel/2005/Aug/1603.html
> 
> So here's my proposal: 
> 
>   Instead of checking for tty->count == 1 in con_open, which we see is
> not reliable.  Just check for tty->driver_data == NULL.
> 
> This should work since it should always be NULL when we need to assign 
> it.  If we switch the events of the race, so that the init_dev went 
> first, the driver_data would not be NULL and would not need to be 
> allocated, because after init_dev tty->count would be greater than 1 
> (this is assuming the case that it is already allocated) and the 
> con_close would not deallocate it.  The tty_sem and console_sem and 
> order of events protect the tty->driver_data but not the tty->count.
> 
> Without the patch, I was able to get the system to BUG on bootup every 
> other time.  With the patch applied, I was able to bootup 6 out of 6 
> times without a single crash.

cool fix. I'm wondering, there's a whole lot of other 'tty->count == 1' 
checks in drivers/char/*.c, could some of those be racy too?

	Ingo

Re: [PATCH] race condition with drivers/char/vt.c (bug in vt_ioctl.c)

[Steven Rostedt]

On Mon, 2005-08-22 at 09:13 +0200, Ingo Molnar wrote:

> 
> cool fix. I'm wondering, there's a whole lot of other 'tty->count == 1' 
> checks in drivers/char/*.c, could some of those be racy too?

I checked them out.  The main problem is that tty->count == 1 is not
reliable in the open function call. Of the 22 files that use tty->count
== 1, only 4 of them (not including vt.c) have a potential race.

Three of the 4, have the race only if an error occurred. 

synclink.c
synclink_cs.c
synclinkmp.c

The code in these three have something like this:


static int open(struct tty_struct *tty, struct file *filp)
{
   ...
	if (/* some error */) {
		retval = /* something bad */
		goto cleanup;
	}
   ...

cleanup:
	if (retval) {
		if (tty->count == 1)
			info->tty = NULL; /* tty layer will release tty struct */
		if(info->count)
			info->count--;
	}

	return retval;
}

So, the info->tty has a chance of not being set to NULL.  I don't know
how bad that is, but this is probably a bug.

The fourth file "ip2main.c" is a little more of a problem.

It's open has the following code:

noblock:

	/* first open - Assign termios structure to port */
	if ( tty->count == 1 ) {
		i2QueueCommands(PTYPE_INLINE, pCh, 0, 2, CMD_CTSFL_DSAB, CMD_RTSFL_DSAB);
		/* Now we must send the termios settings to the loadware */
		set_params( pCh, NULL );
	}

So, there's a chance that tty->count does not equal 1 here when it
should.  The way to fix this would probably be to set driver_data to
NULL with some locking around it, and check that instead.  Like what was
done for vt.c. Since I don't have an ip2 device that I know of, I won't
be fixing this code.

-- Steve

Re: [PATCH] race condition with drivers/char/vt.c (bug in vt_ioctl.c)

[Antonino A. Daplas]

There's a similar report in Kernel Bugzilla

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

I was wondering what's the likelihood of tty->driver_data being NULL in
vt_ioctl but never had the time to do further exploration. Your patch
should fix that bug too.

Tony

Steven Rostedt wrote:
> While debugging Ingo's RT patch, I came accross this race condition.
> The mainline seems to be susceptible to this bug, although it may be 1
> in a 1,000,000 to happen. But those are the nastiest races.
> 
> With debugging information in the RT patch, I was able to reproduce this
> race several times. Enough to be able to debug it. 
> 
> The race is with the tty->driver_data, tty->count and vt.c
> 
> Here's the scoop:
> 
> Process P1 opens a tty:
>   tty_open 
>     --> init_dev
>           sets tty->count to 1
> 
> P1 does what it needs to, and closes the tty.
>   tty_release  (now showing locks)
>     (grabs BKL)
>     --> release_dev
>         --> tty->driver->close ==> con_close (vt.c)
>             (down tty_sem)
>             (aquire console_sem)
>             tty->driver_data = NULL
> 
> Now process P2 opens the console:
>    tty_open
>       (block on tty_sem)
> 
> back to P1
>          (release console_sem)
>          (up tty_sem)
> 
> back to P2
>        (down tty_sem)
>        --> init_dev
>            tty->count++ (tty->count now == 2)
>            (up tty_sem)
>            --> tty->driver->open ==> con_open (vt.c)
>               (aquire console_sem)
>                if (tty->count == 1) (which it does not)
>                     allocate tty->driver_data
>                           (which doesn't happen)
>               (release console_sem)
>   
>      And P2 goes happily along with driver_data == NULL.
> 
> Now in something like vt_ioctl (which I first saw the bug)
> 
> int vt_ioctl(struct tty_struct *tty, struct file * file,
> 	     unsigned int cmd, unsigned long arg)
> {
> 	struct vc_data *vc = (struct vc_data *)tty->driver_data;
> 	struct console_font_op op;	/* used in multiple places here */
> 	struct kbd_struct * kbd;
> 	unsigned int console;
> 	unsigned char ucval;
> 	void __user *up = (void __user *)arg;
> 	int i, perm;
> 	
> 	console = vc->vc_num;
> 
> Where here vc->vc_num could very well be (0)->vc_num.
> 
> I googled a little and found where this may have already happened in the
> main line kernel:
> 
> http://seclists.org/lists/linux-kernel/2005/Aug/1603.html
> 
> So here's my proposal: 
> 
>   Instead of checking for tty->count == 1 in con_open, which we see is
> not reliable.  Just check for tty->driver_data == NULL.
> 
> This should work since it should always be NULL when we need to assign
> it.  If we switch the events of the race, so that the init_dev went
> first, the driver_data would not be NULL and would not need to be
> allocated, because after init_dev tty->count would be greater than 1
> (this is assuming the case that it is already allocated) and the
> con_close would not deallocate it.  The tty_sem and console_sem and
> order of events protect the tty->driver_data but not the tty->count.
> 
> Without the patch, I was able to get the system to BUG on bootup every
> other time.  With the patch applied, I was able to bootup 6 out of 6
> times without a single crash. 
> 
> -- Steve
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> --- linux-2.6.13-rc6-git10/drivers/char/vt.c.orig	2005-08-19 22:51:25.000000000 -0400
> +++ linux-2.6.13-rc6-git10/drivers/char/vt.c	2005-08-19 22:52:22.000000000 -0400
> @@ -2433,7 +2433,7 @@ static int con_open(struct tty_struct *t
>  	int ret = 0;
>  
>  	acquire_console_sem();
> -	if (tty->count == 1) {
> +	if (tty->driver_data == NULL) {
>  		ret = vc_allocate(currcons);
>  		if (ret == 0) {
>  			struct vc_data *vc = vc_cons[currcons].d;
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] race condition with drivers/char/vt.c (bug in vt_ioctl.c)

[Steven Rostedt]

On Tue, 2005-08-23 at 08:08 +0800, Antonino A. Daplas wrote:
> There's a similar report in Kernel Bugzilla
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=4812
> 
> I was wondering what's the likelihood of tty->driver_data being NULL in
> vt_ioctl but never had the time to do further exploration. Your patch
> should fix that bug too.
> 

Yep, that's the bug that is fixed with this patch, so you can close it
out. Well, not until it's added to the mainline (akpm already added it
to -mm).  I wouldn't check for tty->driver_data == NULL, since that
would have been hiding the problem, and not solving it.

-- Steve