Re: NULL dereference in tty_open() [and other bugs there]

From: Jiri Slaby
Date: Wed Oct 05 2011 - 10:28:14 EST

On 10/04/2011 10:05 PM, Dan Carpenter wrote:
> 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.

Thanks, that's very true. It was added by:
commit 0259894c732837c801565d038eaecdcf8fc5bbe7
Author: Jiri Slaby <jslaby@xxxxxxx>
Date: Wed Mar 23 10:48:37 2011 +0100

TTY: fix fail path in tty_open

So instead of leaks, we now crash, hmm.

> 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.

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 <sukadev@xxxxxxxxxx>
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.

suse labs
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at