linux-kernel - RE: [PATCH] tty: hold lock across tty buffer finding and buffer filling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 16 Mar 2012 09:49:30 +0000
From:	"Du, Alek" <alek.du@...el.com>
To:	Jiri Slaby <jslaby@...e.cz>
CC:	Alan Cox <alan@...rguk.ukuu.org.uk>,
	"Tu, Xiaobing" <xiaobing.tu@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"Zhang, Yanmin" <yanmin.zhang@...el.com>,
	"Zuo, Jiao" <jiao.zuo@...el.com>, Jiri Slaby <jirislaby@...il.com>
Subject: RE: [PATCH] tty: hold lock across tty buffer finding and buffer
 filling

The tty structure is protect and not null, the tty->buf.tail is null. Many people think the tty reference count isn't protected and cause this bug, it is not true. :-)

For the flush case, it need acquire the spinlock to free the buffer and put buf.tail to NULL. So this patch will help:

Here is the example place, you can see the __tty_buffer_flush is inside the spinlock.

void tty_buffer_flush(struct tty_struct *tty)
{
        unsigned long flags;
        spin_lock_irqsave(&tty->buf.lock, flags);

        /* If the data is being pushed to the tty layer then we can't
           process it here. Instead set a flag and the flush_to_ldisc
           path will process the flush request before it exits */
        if (test_bit(TTY_FLUSHING, &tty->flags)) {
                set_bit(TTY_FLUSHPENDING, &tty->flags);
                spin_unlock_irqrestore(&tty->buf.lock, flags);
                wait_event(tty->read_wait,
                                test_bit(TTY_FLUSHPENDING, &tty->flags) == 0);
                return;
        } else
                __tty_buffer_flush(tty);
        spin_unlock_irqrestore(&tty->buf.lock, flags);
}



Thanks,
Alek

-----Original Message-----
From: Jiri Slaby [mailto:jirislaby@...il.com] On Behalf Of Jiri Slaby
Sent: Friday, March 16, 2012 5:44 PM
To: Du, Alek
Cc: Alan Cox; Tu, Xiaobing; linux-kernel@...r.kernel.org; gregkh@...uxfoundation.org; Zhang, Yanmin; Zuo, Jiao; Jiri Slaby
Subject: Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

On 03/15/2012 11:14 AM, Du, Alek wrote:
> Alan,
> 
> I agree with you for the case of multiple writers to the tty buffer.
> 
> But there is another case that cause kernel hang: driver is writing data to tty, while app shutdown the port or flush through IOCTL. The buf.tail is set to NULL, here is the log:
> 
> 
> <1>[ 4713.451954] BUG: unable to handle kernel NULL pointer 
> dereference at
> 00000004

The trace only suggests that you don't increment tty reference count in irq properly. If you did that, you would not have tty buffer gone by concurrent shutdown.

For the latter case (flush via ioctl), your patch won't help for tty_prepare_string*, as we return pointer to the buffer which is freed by the flush, right?

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