From: Peter Hurley <peter@hurleysoftware.com>
To: Gianluca Anzolin <gianluca@sottospazio.it>
Cc: gustavo@padovan.org, linux-bluetooth@vger.kernel.org,
marcel@holtmann.org, Greg KH <gregkh@linuxfoundation.org>,
Jiri Slaby <jslaby@suse.cz>
Subject: Re: [PATCH 1/8] Take proper tty references in net/bluetooth/rfcomm/tty.c
Date: Tue, 16 Jul 2013 10:53:47 -0400 [thread overview]
Message-ID: <51E55E7B.8040505@hurleysoftware.com> (raw)
In-Reply-To: <1373661649-1385-1-git-send-email-gianluca@sottospazio.it>
[+cc Greg KH, Jiri Slaby ]
On 07/12/2013 04:40 PM, Gianluca Anzolin wrote:
> In net/bluetooth/rfcomm/tty.c the struct tty_struct is used without taking
> references. This leads to panics because the structure is freed under our feet
> while being used.
If you have a stack trace of a panic which is due to use-after-free
of an rfcomm tty, it should go here. Otherwise, the above sentence should instead
read sometime like, "This may lead to a use-after-free of the rfcomm tty."
Everything else looks ok.
Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
>
> Fix it by taking references properly, using the tty_port_* helpers when
> possible.
>
> Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
> ---
> net/bluetooth/rfcomm/tty.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> index b6e44ad..6c3efb5 100644
> --- a/net/bluetooth/rfcomm/tty.c
> +++ b/net/bluetooth/rfcomm/tty.c
> @@ -333,10 +333,11 @@ static inline unsigned int rfcomm_room(struct rfcomm_dlc *dlc)
> static void rfcomm_wfree(struct sk_buff *skb)
> {
> struct rfcomm_dev *dev = (void *) skb->sk;
> - struct tty_struct *tty = dev->port.tty;
> +
> atomic_sub(skb->truesize, &dev->wmem_alloc);
> - if (test_bit(RFCOMM_TTY_ATTACHED, &dev->flags) && tty)
> - tty_wakeup(tty);
> + if (test_bit(RFCOMM_TTY_ATTACHED, &dev->flags))
> + tty_port_tty_wakeup(&dev->port);
> +
> tty_port_put(&dev->port);
> }
>
> @@ -410,6 +411,7 @@ static int rfcomm_release_dev(void __user *arg)
> {
> struct rfcomm_dev_req req;
> struct rfcomm_dev *dev;
> + struct tty_struct *tty;
>
> if (copy_from_user(&req, arg, sizeof(req)))
> return -EFAULT;
> @@ -429,8 +431,11 @@ static int rfcomm_release_dev(void __user *arg)
> rfcomm_dlc_close(dev->dlc, 0);
>
> /* Shut down TTY synchronously before freeing rfcomm_dev */
> - if (dev->port.tty)
> - tty_vhangup(dev->port.tty);
> + tty = tty_port_tty_get(&dev->port);
> + if (tty) {
> + tty_vhangup(tty);
> + tty_kref_put(tty);
> + }
>
> if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
> rfcomm_dev_del(dev);
> @@ -563,6 +568,7 @@ static void rfcomm_dev_data_ready(struct rfcomm_dlc *dlc, struct sk_buff *skb)
> static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
> {
> struct rfcomm_dev *dev = dlc->owner;
> + struct tty_struct *tty;
> if (!dev)
> return;
>
> @@ -572,7 +578,8 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
> wake_up_interruptible(&dev->wait);
>
> if (dlc->state == BT_CLOSED) {
> - if (!dev->port.tty) {
> + tty = tty_port_tty_get(&dev->port);
> + if (!tty) {
> if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
> /* Drop DLC lock here to avoid deadlock
> * 1. rfcomm_dev_get will take rfcomm_dev_lock
> @@ -591,8 +598,10 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
> tty_port_put(&dev->port);
> rfcomm_dlc_lock(dlc);
> }
> - } else
> - tty_hangup(dev->port.tty);
> + } else {
> + tty_hangup(tty);
> + tty_kref_put(tty);
> + }
> }
> }
>
> @@ -604,10 +613,8 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
>
> BT_DBG("dlc %p dev %p v24_sig 0x%02x", dlc, dev, v24_sig);
>
> - if ((dev->modem_status & TIOCM_CD) && !(v24_sig & RFCOMM_V24_DV)) {
> - if (dev->port.tty && !C_CLOCAL(dev->port.tty))
> - tty_hangup(dev->port.tty);
> - }
> + if ((dev->modem_status & TIOCM_CD) && !(v24_sig & RFCOMM_V24_DV))
> + tty_port_tty_hangup(&dev->port, true);
>
> dev->modem_status =
> ((v24_sig & RFCOMM_V24_RTC) ? (TIOCM_DSR | TIOCM_DTR) : 0) |
> @@ -674,7 +681,7 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
>
> rfcomm_dlc_lock(dlc);
> tty->driver_data = dev;
> - dev->port.tty = tty;
> + tty_port_tty_set(&dev->port, tty);
> rfcomm_dlc_unlock(dlc);
> set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
>
> @@ -742,7 +749,7 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
>
> rfcomm_dlc_lock(dev->dlc);
> tty->driver_data = NULL;
> - dev->port.tty = NULL;
> + tty_port_tty_set(&dev->port, NULL);
> rfcomm_dlc_unlock(dev->dlc);
>
> if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags)) {
>
prev parent reply other threads:[~2013-07-16 14:53 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-12 20:40 [PATCH 1/8] Take proper tty references in net/bluetooth/rfcomm/tty.c Gianluca Anzolin
2013-07-12 20:40 ` [PATCH 2/8] Move functions before the definition of rfcomm_port_ops Gianluca Anzolin
2013-07-16 15:14 ` Peter Hurley
2013-07-12 20:40 ` [PATCH 3/8] Move device initialization and shutdown to tty_port_operations Gianluca Anzolin
2013-07-16 20:48 ` Peter Hurley
2013-07-20 7:10 ` Gianluca Anzolin
2013-07-20 14:11 ` Peter Hurley
2013-07-21 8:08 ` Gianluca Anzolin
2013-07-21 17:04 ` Peter Hurley
2013-07-21 17:31 ` Gianluca Anzolin
2013-07-12 20:40 ` [PATCH 4/8] Move tty initialization and cleanup out of open/close Gianluca Anzolin
2013-07-16 19:07 ` Peter Hurley
2013-07-12 20:40 ` [PATCH 5/8] Use the tty_port_* functions in tty_open/tty_close/tty_hangup Gianluca Anzolin
2013-07-16 20:51 ` Peter Hurley
2013-07-17 8:03 ` Gianluca Anzolin
2013-07-12 20:40 ` [PATCH 6/8] Fix the reference counting of tty_port Gianluca Anzolin
2013-07-17 14:02 ` Peter Hurley
2013-07-17 17:05 ` Gianluca Anzolin
2013-07-17 18:10 ` Peter Hurley
2013-07-18 12:45 ` Peter Hurley
2013-07-18 14:13 ` Gianluca Anzolin
2013-07-18 15:19 ` Peter Hurley
2013-07-12 20:40 ` [PATCH 7/8] Avoid a circular dependency between dev and dev->dlc Gianluca Anzolin
2013-07-12 20:40 ` [PATCH 8/8] Add module_put in rfcomm_dev_add error path Gianluca Anzolin
2013-07-17 15:20 ` Peter Hurley
2013-07-16 14:53 ` Peter Hurley [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51E55E7B.8040505@hurleysoftware.com \
--to=peter@hurleysoftware.com \
--cc=gianluca@sottospazio.it \
--cc=gregkh@linuxfoundation.org \
--cc=gustavo@padovan.org \
--cc=jslaby@suse.cz \
--cc=linux-bluetooth@vger.kernel.org \
--cc=marcel@holtmann.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).