Sascha Levin reported that the syzkaller fuzzer triggered a WARNING in
ser_gigaset (see https://lkml.kernel.org/g/[email protected] ). It
turned out that ser_gigaset has always deallocated its platform device
structure incorrectly. Tilman submitted the patch that fixes that (3/4) and a
related cleanup (4/4).
Tilman also submitted a minor cleanup of some NULL checks (1/4) that prompted
Alan to turn those checks into WARN_ONs (2/4). If no one hits these WARN_ONs in
the next couple of releases these WARN_ONs should be removed.
Alan Cox (1):
ser_gigaset: turn nonsense checks into WARN_ON
Tilman Schmidt (3):
ser_gigaset: fix up NULL checks
ser_gigaset: fix deallocation of platform device structure
ser_gigaset: remove unnecessary kfree() calls from release method
drivers/isdn/gigaset/ser-gigaset.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
--
2.4.3
From: Tilman Schmidt <[email protected]>
Commit f34d7a5b7010 ("tty: The big operations rework") changed
tty->driver to tty->ops but left NULL checks for tty->driver untouched.
Fix.
Signed-off-by: Tilman Schmidt <[email protected]>
[pebolle: removed Fixes tag]
Signed-off-by: Paul Bolle <[email protected]>
---
drivers/isdn/gigaset/ser-gigaset.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 375be509e95f..d8771b5d6904 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -67,7 +67,7 @@ static int write_modem(struct cardstate *cs)
struct sk_buff *skb = bcs->tx_skb;
int sent = -EOPNOTSUPP;
- if (!tty || !tty->driver || !skb)
+ if (!tty || !tty->ops || !skb)
return -EINVAL;
if (!skb->len) {
@@ -109,7 +109,7 @@ static int send_cb(struct cardstate *cs)
unsigned long flags;
int sent = 0;
- if (!tty || !tty->driver)
+ if (!tty || !tty->ops)
return -EFAULT;
cb = cs->cmdbuf;
@@ -432,7 +432,7 @@ static int gigaset_set_modem_ctrl(struct cardstate *cs, unsigned old_state,
struct tty_struct *tty = cs->hw.ser->tty;
unsigned int set, clear;
- if (!tty || !tty->driver || !tty->ops->tiocmset)
+ if (!tty || !tty->ops || !tty->ops->tiocmset)
return -EINVAL;
set = new_state & ~old_state;
clear = old_state & ~new_state;
--
2.4.3
From: Alan Cox <[email protected]>
These checks do nothing useful to protect the code from races. On the
other hand if the old code has been masking a real bug we would like to
know about it.
The check for tiocmset is kept because it is valid for a tty driver to
have a NULL tiocmset method. That in itself is probably a mistake given
modern coding practices - but needs fixing in the tty layer.
Signed-off-by: Alan Cox <[email protected]>
Acked-by: Tilman Schmidt <[email protected]>
Signed-off-by: Paul Bolle <[email protected]>
---
drivers/isdn/gigaset/ser-gigaset.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index d8771b5d6904..8e21f6afa832 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -67,8 +67,7 @@ static int write_modem(struct cardstate *cs)
struct sk_buff *skb = bcs->tx_skb;
int sent = -EOPNOTSUPP;
- if (!tty || !tty->ops || !skb)
- return -EINVAL;
+ WARN_ON(!tty || !tty->ops || !skb);
if (!skb->len) {
dev_kfree_skb_any(skb);
@@ -109,8 +108,7 @@ static int send_cb(struct cardstate *cs)
unsigned long flags;
int sent = 0;
- if (!tty || !tty->ops)
- return -EFAULT;
+ WARN_ON(!tty || !tty->ops);
cb = cs->cmdbuf;
if (!cb)
@@ -432,7 +430,9 @@ static int gigaset_set_modem_ctrl(struct cardstate *cs, unsigned old_state,
struct tty_struct *tty = cs->hw.ser->tty;
unsigned int set, clear;
- if (!tty || !tty->ops || !tty->ops->tiocmset)
+ WARN_ON(!tty || !tty->ops);
+ /* tiocmset is an optional tty driver method */
+ if (!tty->ops->tiocmset)
return -EINVAL;
set = new_state & ~old_state;
clear = old_state & ~new_state;
--
2.4.3
From: Tilman Schmidt <[email protected]>
When shutting down the device, the struct ser_cardstate must not be
kfree()d immediately after the call to platform_device_unregister()
since the embedded struct platform_device is still in use.
Move the kfree() call to the release method instead.
Signed-off-by: Tilman Schmidt <[email protected]>
Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
Reported-by: Sasha Levin <[email protected]>
Signed-off-by: Paul Bolle <[email protected]>
---
drivers/isdn/gigaset/ser-gigaset.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 8e21f6afa832..635baaf34e95 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -368,19 +368,23 @@ static void gigaset_freecshw(struct cardstate *cs)
tasklet_kill(&cs->write_tasklet);
if (!cs->hw.ser)
return;
- dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
platform_device_unregister(&cs->hw.ser->dev);
- kfree(cs->hw.ser);
- cs->hw.ser = NULL;
}
static void gigaset_device_release(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
+ struct cardstate *cs = dev_get_drvdata(dev);
/* adapted from platform_device_release() in drivers/base/platform.c */
kfree(dev->platform_data);
kfree(pdev->resource);
+
+ if (!cs)
+ return;
+ dev_set_drvdata(dev, NULL);
+ kfree(cs->hw.ser);
+ cs->hw.ser = NULL;
}
/*
--
2.4.3
From: Tilman Schmidt <[email protected]>
device->platform_data and platform_device->resource are never used
and remain NULL through their entire life. Drops the kfree() calls
for them from the device release method.
Signed-off-by: Tilman Schmidt <[email protected]>
Signed-off-by: Paul Bolle <[email protected]>
---
drivers/isdn/gigaset/ser-gigaset.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 635baaf34e95..2a506fe0c8a4 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -373,13 +373,8 @@ static void gigaset_freecshw(struct cardstate *cs)
static void gigaset_device_release(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
struct cardstate *cs = dev_get_drvdata(dev);
- /* adapted from platform_device_release() in drivers/base/platform.c */
- kfree(dev->platform_data);
- kfree(pdev->resource);
-
if (!cs)
return;
dev_set_drvdata(dev, NULL);
--
2.4.3
From: Paul Bolle <[email protected]>
Date: Tue, 15 Dec 2015 18:11:27 +0100
> Sascha Levin reported that the syzkaller fuzzer triggered a WARNING in
> ser_gigaset (see https://lkml.kernel.org/g/[email protected] ). It
> turned out that ser_gigaset has always deallocated its platform device
> structure incorrectly. Tilman submitted the patch that fixes that (3/4) and a
> related cleanup (4/4).
>
> Tilman also submitted a minor cleanup of some NULL checks (1/4) that prompted
> Alan to turn those checks into WARN_ONs (2/4). If no one hits these WARN_ONs in
> the next couple of releases these WARN_ONs should be removed.
Series applied, thanks Paul.