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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 14 Jan 2020 07:17:51 -0800 From: Eric Dumazet <eric.dumazet@...il.com> To: Richard Palethorpe <rpalethorpe@...e.com>, linux-can@...r.kernel.org Cc: syzbot+017e491ae13c0068598a@...kaller.appspotmail.com, Wolfgang Grandegger <wg@...ndegger.com>, Marc Kleine-Budde <mkl@...gutronix.de>, "David S. Miller" <davem@...emloft.net>, Tyler Hall <tylerwhall@...il.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, syzkaller@...glegroups.com Subject: Re: [PATCH] can, slip: Protect tty->disc_data access with RCU On 1/14/20 6:32 AM, Richard Palethorpe wrote: > write_wakeup can happen in parallel with close where tty->disc_data is set > to NULL. So we a) need to check if tty->disc_data is NULL and b) ensure it > is an atomic operation. Otherwise accessing tty->disc_data could result in > a NULL pointer deref or access to some random location. > > This problem was found by Syzkaller on slcan, but the same issue appears to > exist in slip where slcan was copied from. > > A fix which didn't use RCU was posted by Hillf Danton. > > Fixes: 661f7fda21b1 ("slip: Fix deadlock in write_wakeup") > Fixes: a8e83b17536a ("slcan: Port write_wakeup deadlock fix from slip") > Reported-by: syzbot+017e491ae13c0068598a@...kaller.appspotmail.com > Signed-off-by: Richard Palethorpe <rpalethorpe@...e.com> > Cc: Wolfgang Grandegger <wg@...ndegger.com> > Cc: Marc Kleine-Budde <mkl@...gutronix.de> > Cc: "David S. Miller" <davem@...emloft.net> > Cc: Tyler Hall <tylerwhall@...il.com> > Cc: netdev@...r.kernel.org > Cc: linux-kernel@...r.kernel.org > Cc: syzkaller@...glegroups.com > --- > > Note, that mabye RCU should also applied to receive_buf as that also happens > in interrupt context. So if the pointer assignment is split by the compiler > then sl may point somewhere unexpected? > > drivers/net/can/slcan.c | 11 +++++++++-- > drivers/net/slip/slip.c | 11 +++++++++-- > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c > index 2e57122f02fb..ee029aae69d4 100644 > --- a/drivers/net/can/slcan.c > +++ b/drivers/net/can/slcan.c > @@ -344,7 +344,14 @@ static void slcan_transmit(struct work_struct *work) > */ > static void slcan_write_wakeup(struct tty_struct *tty) > { > - struct slcan *sl = tty->disc_data; > + struct slcan *sl; > + > + rcu_read_lock(); > + sl = rcu_dereference(tty->disc_data); > + rcu_read_unlock(); This rcu_read_lock()/rcu_read_unlock() pair is not protecting anything. Right after rcu_read_unlock(), sl validity can not be guaranteed. > + > + if (!sl) > + return; > > schedule_work(&sl->tx_work); > } > @@ -644,7 +651,7 @@ static void slcan_close(struct tty_struct *tty) > return; > > spin_lock_bh(&sl->lock); > - tty->disc_data = NULL; > + rcu_assign_pointer(tty->disc_data, NULL); > sl->tty = NULL; > spin_unlock_bh(&sl->lock); Where is the rcu grace period before freeing enforced ? > > diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c > index 2a91c192659f..dfed9f0b8646 100644 > --- a/drivers/net/slip/slip.c > +++ b/drivers/net/slip/slip.c > @@ -452,7 +452,14 @@ static void slip_transmit(struct work_struct *work) > */ > static void slip_write_wakeup(struct tty_struct *tty) > { > - struct slip *sl = tty->disc_data; > + struct slip *sl; > + > + rcu_read_lock(); > + sl = rcu_dereference(tty->disc_data); > + rcu_read_unlock(); Same here. > + > + if (!sl) > + return; > > schedule_work(&sl->tx_work); > } > @@ -882,7 +889,7 @@ static void slip_close(struct tty_struct *tty) > return; > > spin_lock_bh(&sl->lock); > - tty->disc_data = NULL; > + rcu_assign_pointer(tty->disc_data, NULL); > sl->tty = NULL; > spin_unlock_bh(&sl->lock); > >
Powered by blists - more mailing lists