[PATCH 1/1] usb: cdc-acm: Fix acm_tty_hangup() vs. acm_tty_close() race

[PATCH 1/1] usb: cdc-acm: Fix acm_tty_hangup() vs. acm_tty_close() race

[Thilo-Alexander Ginkel]

There is a race condition involving acm_tty_hangup() and acm_tty_close() where
hangup() would attempt to access tty->driver_data without proper locking and
NULL checking after close() has potentially already set it to NULL.
One possibility to (sporadically) trigger this behavior is to perform a
suspend/resume cycle with a running WWAN data connection.

This patch addresses the issue by introducing a NULL check for
tty->driver_data in acm_tty_hangup() protected by open_mutex and exiting
gracefully when hangup() is invoked on a device that has already been closed.

Signed-off-by: Thilo-Alexander Ginkel <thilo@ginkel.com>
---
 drivers/usb/class/cdc-acm.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index a8078d0..97f2e58 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -554,10 +554,18 @@ static void acm_port_down(struct acm *acm)
 
 static void acm_tty_hangup(struct tty_struct *tty)
 {
-	struct acm *acm = tty->driver_data;
-	tty_port_hangup(&acm->port);
+	struct acm *acm;
+
 	mutex_lock(&open_mutex);
+	acm = tty->driver_data;
+
+	if (!acm)
+		goto out;
+
+	tty_port_hangup(&acm->port);
 	acm_port_down(acm);
+
+out:
 	mutex_unlock(&open_mutex);
 }
 
-- 
1.7.5.4

Re: [PATCH 1/1] usb: cdc-acm: Fix acm_tty_hangup() vs. acm_tty_close() race

[Thilo-Alexander Ginkel]

Hi there,

On Sat, Dec 17, 2011 at 10:55, Thilo-Alexander Ginkel <thilo@ginkel.com> wrote:
> There is a race condition involving acm_tty_hangup() and acm_tty_close() where
> hangup() would attempt to access tty->driver_data without proper locking and
> NULL checking after close() has potentially already set it to NULL.
> One possibility to (sporadically) trigger this behavior is to perform a
> suspend/resume cycle with a running WWAN data connection.
>
> This patch addresses the issue by introducing a NULL check for
> tty->driver_data in acm_tty_hangup() protected by open_mutex and exiting
> gracefully when hangup() is invoked on a device that has already been closed.

just wondering: Is there any chance that this patch will make it into
3.3? I would really appreciate that as the bug it fixes is pretty
annoying (and potentially causes data loss when the laptop locks up on
resume) and I am sure that I am not the only one affected by it (at
least all Lenovo T420s with an Ericsson F5521gw WWAN modem should be
similarly affected).

If you would like to see any changes to the code or commit message
before considering the patch for inclusion, please let me know.

Thanks,
Thilo

Re: [PATCH 1/1] usb: cdc-acm: Fix acm_tty_hangup() vs. acm_tty_close() race

[Greg KH]

On Sun, Dec 25, 2011 at 10:43:09PM +0100, Thilo-Alexander Ginkel wrote:
> Hi there,
> 
> On Sat, Dec 17, 2011 at 10:55, Thilo-Alexander Ginkel <thilo@ginkel.com> wrote:
> > There is a race condition involving acm_tty_hangup() and acm_tty_close() where
> > hangup() would attempt to access tty->driver_data without proper locking and
> > NULL checking after close() has potentially already set it to NULL.
> > One possibility to (sporadically) trigger this behavior is to perform a
> > suspend/resume cycle with a running WWAN data connection.
> >
> > This patch addresses the issue by introducing a NULL check for
> > tty->driver_data in acm_tty_hangup() protected by open_mutex and exiting
> > gracefully when hangup() is invoked on a device that has already been closed.
> 
> just wondering: Is there any chance that this patch will make it into
> 3.3? I would really appreciate that as the bug it fixes is pretty
> annoying (and potentially causes data loss when the laptop locks up on
> resume) and I am sure that I am not the only one affected by it (at
> least all Lenovo T420s with an Ericsson F5521gw WWAN modem should be
> similarly affected).
> 
> If you would like to see any changes to the code or commit message
> before considering the patch for inclusion, please let me know.

As this isn't going to be an issue with 3.3 (from what I can tell, the
code is correctly reworked in the patch set queued up for 3.3), I'll
submit this for only the 3.2 and older kernel -stable releases, as that
is the only place this should be relevant for.

thanks,

greg k-h