NULL dereference in tty_open()

There is a NULL dereference here. It was artificially triggered so not a huge priority.

drivers/tty/tty_io.c
1893 retval = tty_add_file(tty, filp);
1894 if (retval) {

  1895                  tty_unlock();
  1896                  tty_release(inode, filp);
  1897                  return retval;

1898 }

tty_add_file() is supposed to setup filp->private_data but the allocation fails. In tty_release() we call file_tty(filp), __tty_fasync() and tty_del_file() which dereference filp->private_data and Oops.

I looked at ptmx_open() to see how the error handling was done there. That function only calls tty_release() if tty_add_file() succeeds, so maybe we could just call devpts_kill_index() here and remove the tty_release()? I don't know the code well enough to say.

regards,
dan carpenter

Thanks, that's very true. It was added by:
commit 0259894c732837c801565d038eaecdcf8fc5bbe7

Author: Jiri Slaby <[email protected]>
Date:   Wed Mar 23 10:48:37 2011 +0100

TTY: fix fail path in tty_open
=====

So instead of leaks, we now crash, hmm.

No, that won't work. We need to revert all the changes done in tty_reopen/tty_init_dev. Yes, ptmx_open looks broken to me too because the tty is not properly freed.

What is worse, the tty_open code seems to be broken more than that.
* when tty_driver_lookup_tty fails in tty_open, we don't drop a reference to the tty driver.
* tty lookup looks broken for the current console. Look:

As of:
commit 4a2b5fddd53b80efcb3266ee36e23b8de28e761a

Author: Sukadev Bhattiprolu <[email protected]>
Date:   Mon Oct 13 10:42:49 2008 +0100

Move tty lookup/reopen to caller
=====

The code looks like:
struct tty_struct *tty = NULL;
...
if (device == MKDEV(TTYAUX_MAJOR, 0)) {
tty = get_current_tty(); // ==== tty is not NULL
...
tty_kref_put(tty); // ==== tty is dropped goto got_driver;
}
... // ============== POINT 1 (see below) if (tty) {
retval = tty_reopen(tty);
...
}
=====

Previously we called tty_driver_lookup_tty unconditionally at POINT 1. Now we pass a tty structure to tty_reopen for which we dropped a reference count. I don't think this was intentional, right?

Back to the point of your email. I have a patch which splits tty_add_file into:
* tty_alloc_file intented to be called earlier in the open functions, so we don't need to rollback
* tty_add_file which doesn't fail. It sets up the structure and adds it to the list.

Otherwise we would need to split tty_release into smaller functions somehow. We would need at least decreasing refcounts, checking tty count for zero and freeing tty.

I will submit the simpler way above (tty_add_file split) along with fixes for the other 3 bugs (ptmx_open, tty driver refcount, tty_reopen of an unreferenced tty) after I test them all a bit.

thanks,

js
suse labs

When tty_driver_lookup_tty fails in tty_open, we forget to drop a reference to the tty driver. This was added by commit 4a2b5fddd5 (Move tty lookup/reopen to caller).

Fix that by adding tty_driver_kref_put to the fail path.

I will refactor the code later. This is for the ease of backporting to stable.

Introduced-in: v2.6.28-rc2

---
 drivers/tty/tty_io.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 4f1fc81..54b3cc4 100644
@@ -1872,6 +1872,7 @@ got_driver:
 		if (IS_ERR(tty)) {
 			tty_unlock();
 			mutex_unlock(&tty_mutex);
+			tty_driver_kref_put(driver);
 			return PTR_ERR(tty);
 		}
 	}

1.7.7

If tty_add_file fails at the point it is now, we have to revert all the changes we did to the tty. It means either decrease all refcounts if this was a tty reopen or delete the tty if it was newly allocated.

There was a try to fix this in v3.0-rc2 using tty_release in 0259894c7 (TTY: fix fail path in tty_open). But instead it introduced a NULL dereference. It's because tty_release dereferences filp->private_data, but that one is set even in our tty_add_file. And when tty_add_file fails, it's still NULL/garbage. Hence tty_release cannot be called there.

To circumvent the original leak (and the current NULL deref) we split tty_add_file into two functions, making the latter non-failing. In that case we may do the former early in open, where handling failures is easy. The latter stays as it is now. So there is no change in functionality.

The original bug (leak) was introduced by f573bd176 (tty: Remove __GFP_NOFAIL from tty_add_file()). Thanks Dan for reporting this.

Later, we may split tty_release into more functions and call only some of them in this fail path instead. (If at all possible.)

Introduced-in: v2.6.37-rc2

---
 drivers/tty/pty.c    |   16 +++++++++++-----
 drivers/tty/tty_io.c |   47 +++++++++++++++++++++++++++++++++++------------
 include/linux/tty.h  |    4 +++-
 3 files changed, 49 insertions(+), 18 deletions(-)

index e809e9d..696e851 100644
@@ -670,12 +670,18 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 
 	nonseekable_open(inode, filp);
 
+	retval = tty_alloc_file(filp);
+	if (retval)
+		return retval;
+
 	/* find a device that is not in use. */
 	tty_lock();
 	index = devpts_new_index(inode);
 	tty_unlock();
-	if (index < 0)
-		return index;
+	if (index < 0) {
+		retval = index;
+		goto err_file;
+	}
 
 	mutex_lock(&tty_mutex);
 	tty_lock();

@@ -689,9 +695,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 
 	set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
 
-	retval = tty_add_file(tty, filp);
-	if (retval)
-		goto out;
+	tty_add_file(tty, filp);
 
 	retval = devpts_pty_new(inode, tty->link);
 	if (retval)

@@ -710,6 +714,8 @@ out2:
 out:
 	devpts_kill_index(inode, index);
 	tty_unlock();
+err_file:
+	tty_free_file(filp);
 	return retval;
 }
 

index 6af44ab..ac275c2 100644
@@ -479,7 +479,9 @@ extern void proc_clear_tty(struct task_struct *p);
 extern struct tty_struct *get_current_tty(void);
 extern void tty_default_fops(struct file_operations *fops);
 extern struct tty_struct *alloc_tty_struct(void);
-extern int tty_add_file(struct tty_struct *tty, struct file *file);
+extern int tty_alloc_file(struct file *file);
+extern void tty_add_file(struct tty_struct *tty, struct file *file);
+extern void tty_free_file(struct file *file);
 extern void free_tty_struct(struct tty_struct *tty);
 extern void initialize_tty_struct(struct tty_struct *tty,
 		struct tty_driver *driver, int idx);

1.7.7

Mistakenly, commit 64ba3dc3143d (tty: never hold BTM while getting tty_mutex) switched one fail path in ptmx_open to not free the newly allocated tty.

Fix that by jumping to the appropriate place. And rename the labels so that it's clear what is going on there.

Introduced-in: v2.6.36-rc2

---
 drivers/tty/pty.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

index 696e851..e18604b 100644
@@ -699,15 +699,15 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 
 	retval = devpts_pty_new(inode, tty->link);
 	if (retval)
-		goto out1;
+		goto err_release;
 
 	retval = ptm_driver->ops->open(tty, filp);
 	if (retval)
-		goto out2;
-out1:
+		goto err_release;
+
 	tty_unlock();
-	return retval;
-out2:
+	return 0;
+err_release:
 	tty_unlock();
 	tty_release(inode, filp);
 	return retval;

1.7.7

Ok, I have checked that this patch returns the code into the state before my broken patch, that looks good.

I think I made the same mistake trying to understand your patch that I made originally when I introduced the bug, which was to confuse the lifetime of the pty master with the lifetime of the slave device, so I thought that tty_release should not be called if we never called ptm_driver->ops->open(), but that's actually a different device.

Arnd

Commit 4a2b5fddd5 (Move tty lookup/reopen to caller) made the call to tty_driver_lookup_tty conditional in tty_open. It doesn't look like it was an intention. Or if it was, it was not documented in the changelog and the code now looks weird. For example there would be no need to remember the tty driver and tty index. Further the condition depends on a tty which we drop a reference of already.

If I'm looking correctly, this should not matter thanks to the locking currently done there. Thus, tty_driver->ttys[idx] cannot change under our hands. But anyway, it makes sense to change that to the old behaviour.

Introduced-in: v2.6.28-rc2

---
 drivers/tty/tty_io.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

index 1a890e2..3283d0a 100644
@@ -1820,7 +1820,7 @@ int tty_release(struct inode *inode, struct file *filp)
 
 static int tty_open(struct inode *inode, struct file *filp)
 {
-	struct tty_struct *tty = NULL;
+	struct tty_struct *tty;
 	int noctty, retval;
 	struct tty_driver *driver;
 	int index;

@@ -1891,17 +1891,14 @@ retry_open:
 		return -ENODEV;
 	}
 got_driver:
-	if (!tty) {
-		/* check whether we're reopening an existing tty */
-		tty = tty_driver_lookup_tty(driver, inode, index);
-
-		if (IS_ERR(tty)) {
-			tty_unlock();
-			mutex_unlock(&tty_mutex);
-			tty_driver_kref_put(driver);
-			tty_free_file(filp);
-			return PTR_ERR(tty);
-		}
+	/* check whether we're reopening an existing tty */
+	tty = tty_driver_lookup_tty(driver, inode, index);
+	if (IS_ERR(tty)) {
+		tty_unlock();
+		mutex_unlock(&tty_mutex);
+		tty_driver_kref_put(driver);
+		tty_free_file(filp);
+		return PTR_ERR(tty);
 	}
 
 	if (tty) {

1.7.7

Well, this doesn't work for ptys. devpts lookup code expects an inode to be one of devpts filesystem (/dev/pts/*), not something on tmpfs like /dev/tty. So it looks like the change was intentional, but very undocumented and leaving there some unused code.

Now, I don't know what to do with the tty being around with a dropped reference. Maybe nothing in the presence of the BTM which I *think* prevents tty to go now.

Also I have just found that having ptm_unix98_lookup is pointless. For master, we have ptmx_open which doesn't call lookup. And if it did, it would trigger BUG_ON in devpts_get_tty called from ptm_unix98_lookup.

thanks,

js

Ccing H.Peter Anvin.

Jiri Slaby [[email protected]] wrote:

| On 10/12/2011 11:32 AM, Jiri Slaby wrote:
| > Commit 4a2b5fddd5 (Move tty lookup/reopen to caller) made the call to
| > tty_driver_lookup_tty conditional in tty_open. It doesn't look like it
| > was an intention. Or if it was, it was not documented in the changelog
| > and the code now looks weird. For example there would be no need to
| > remember the tty driver and tty index. Further the condition depends
| > on a tty which we drop a reference of already.
| > 
| > If I'm looking correctly, this should not matter thanks to the locking
| > currently done there. Thus, tty_driver->ttys[idx] cannot change under
| > our hands. But anyway, it makes sense to change that to the old
| > behaviour.
| 
| Well, this doesn't work for ptys. devpts lookup code expects an inode to
| be one of devpts filesystem (/dev/pts/*), not something on tmpfs like
| /dev/tty. So it looks like the change was intentional, but very
| undocumented and leaving there some unused code.

Yes this was intentional - even though the tty_driver_lookup() was unconditional in tty_init_dev() I believe it did not do anything useful when called from ptmx_open(). ptmx_open() would be setting up a new pty and the lookup would not find a tty.

Would the following change to tty_open() help ?

got_driver:
+ /* check if we are opening a new pty or reopening an existing tty */

        if (!tty) {
-	/* check whether we're reopening an existing tty */
                tty = tty_driver_lookup_tty(driver, inode, index);

I am not sure about the unused code you refer to above. Can you please clarify ?

Sukadev

Fixing Alan's address. Somehow I put there a RH (defunct) one.

Yes, I'm not arguing against moving the code from tty_init_dev to tty_open change. That is perfectly OK.

What I mind is that now we do:
=====
tty = get_current_tty();
if (!tty)

        return -ENXIO;

driver = tty_driver_kref_get(tty->driver); /* ZZZ */
index = tty->index; /* ZZZ */
...
tty_kref_put(tty); /* XXX */
goto got_driver;
...
got_driver:
if (!tty) { /* YYY */
}
=====

But at the point of YYY, the tty may be invalid due to reference drop at XXX. I *hope* it is not the case thanks to the current locking there (namely BTM) but I'm really *not sure*. And if it is the case, there should definitely be a note. (Or better the reference should be held while necessary.)

No, it doesn't matter now. I would prefer a description of the change to be a part of the commit log. And it missed such a notice.

It is index and driver assigned in the branch above (see ZZZ). When we have a live tty (which I'm not sure we have -- see above), it is guaranteed that the driver is reff'ed. And we need neither driver nor index when we have such a tty.

thanks,

js

Jiri Slaby [[email protected]] wrote:

| When tty_driver_lookup_tty fails in tty_open, we forget to drop a
| reference to the tty driver. This was added by commit 4a2b5fddd5 (Move
| tty lookup/reopen to caller).
| 
| Fix that by adding tty_driver_kref_put to the fail path.

Arg. Sorry about that. Thanks for spotting/fixing it.

| 
| I will refactor the code later. This is for the ease of backporting to
| stable.
| 
| Introduced-in: v2.6.28-rc2
| Signed-off-by: Jiri Slaby <[email protected]>
| Cc: [email protected]
| Cc: Sukadev Bhattiprolu <[email protected]>
| Cc: Alan Cox <[email protected]>
| Cc: Greg Kroah-Hartman <[email protected]>