All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] vt: selection, push console lock down
@ 2020-02-28 11:54 Jiri Slaby
  2020-02-28 11:54 ` [PATCH 2/2] vt: selection, push sel_lock up Jiri Slaby
  0 siblings, 1 reply; 5+ messages in thread
From: Jiri Slaby @ 2020-02-28 11:54 UTC (permalink / raw)
  To: gregkh; +Cc: linux-serial, linux-kernel, Jiri Slaby

We need to nest the console lock in sel_lock, so we have to push it down
a bit. Fortunately, the callers of set_selection_* just lock the console
lock around the function call. So moving it down is easy.

In the next patch, we switch the order.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/staging/speakup/selection.c |  2 --
 drivers/tty/vt/selection.c          | 13 ++++++++++++-
 drivers/tty/vt/vt.c                 |  2 --
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
index a8b4d0c5ab7e..032f3264fba1 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -51,9 +51,7 @@ static void __speakup_set_selection(struct work_struct *work)
 		goto unref;
 	}
 
-	console_lock();
 	set_selection_kernel(&sel, tty);
-	console_unlock();
 
 unref:
 	tty_kref_put(tty);
diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 714992693974..5b0b897396eb 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -186,7 +186,7 @@ int set_selection_user(const struct tiocl_selection __user *sel,
 	return set_selection_kernel(&v, tty);
 }
 
-int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
+static int __set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
 {
 	struct vc_data *vc = vc_cons[fg_console].d;
 	int new_sel_start, new_sel_end, spc;
@@ -348,6 +348,17 @@ int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
 	mutex_unlock(&sel_lock);
 	return ret;
 }
+
+int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
+{
+	int ret;
+
+	console_lock();
+	ret = __set_selection_kernel(v, tty);
+	console_unlock();
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(set_selection_kernel);
 
 /* Insert the contents of the selection buffer into the
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 8fa059ec6cc8..c4d75edde923 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3047,10 +3047,8 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
 	switch (type)
 	{
 		case TIOCL_SETSEL:
-			console_lock();
 			ret = set_selection_user((struct tiocl_selection
 						 __user *)(p+1), tty);
-			console_unlock();
 			break;
 		case TIOCL_PASTESEL:
 			ret = paste_selection(tty);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 2/2] vt: selection, push sel_lock up
  2020-02-28 11:54 [PATCH 1/2] vt: selection, push console lock down Jiri Slaby
@ 2020-02-28 11:54 ` Jiri Slaby
  2020-02-28 12:03   ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: Jiri Slaby @ 2020-02-28 11:54 UTC (permalink / raw)
  To: gregkh
  Cc: linux-serial, linux-kernel, Jiri Slaby, syzbot+26183d9746e62da329b8

sel_lock cannot nest in the console lock. Thanks to syzkaller, the
kernel states firmly:

> WARNING: possible circular locking dependency detected
> 5.6.0-rc3-syzkaller #0 Not tainted
> ------------------------------------------------------
> syz-executor.4/20336 is trying to acquire lock:
> ffff8880a2e952a0 (&tty->termios_rwsem){++++}, at: tty_unthrottle+0x22/0x100 drivers/tty/tty_ioctl.c:136
>
> but task is already holding lock:
> ffffffff89462e70 (sel_lock){+.+.}, at: paste_selection+0x118/0x470 drivers/tty/vt/selection.c:374
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 (sel_lock){+.+.}:
>        mutex_lock_nested+0x1b/0x30 kernel/locking/mutex.c:1118
>        set_selection_kernel+0x3b8/0x18a0 drivers/tty/vt/selection.c:217
>        set_selection_user+0x63/0x80 drivers/tty/vt/selection.c:181
>        tioclinux+0x103/0x530 drivers/tty/vt/vt.c:3050
>        vt_ioctl+0x3f1/0x3a30 drivers/tty/vt/vt_ioctl.c:364

This is ioctl(TIOCL_SETSEL).
Locks held on the path: console_lock -> sel_lock

> -> #1 (console_lock){+.+.}:
>        console_lock+0x46/0x70 kernel/printk/printk.c:2289
>        con_flush_chars+0x50/0x650 drivers/tty/vt/vt.c:3223
>        n_tty_write+0xeae/0x1200 drivers/tty/n_tty.c:2350
>        do_tty_write drivers/tty/tty_io.c:962 [inline]
>        tty_write+0x5a1/0x950 drivers/tty/tty_io.c:1046

This is write().
Locks held on the path: termios_rwsem -> console_lock

> -> #0 (&tty->termios_rwsem){++++}:
>        down_write+0x57/0x140 kernel/locking/rwsem.c:1534
>        tty_unthrottle+0x22/0x100 drivers/tty/tty_ioctl.c:136
>        mkiss_receive_buf+0x12aa/0x1340 drivers/net/hamradio/mkiss.c:902
>        tty_ldisc_receive_buf+0x12f/0x170 drivers/tty/tty_buffer.c:465
>        paste_selection+0x346/0x470 drivers/tty/vt/selection.c:389
>        tioclinux+0x121/0x530 drivers/tty/vt/vt.c:3055
>        vt_ioctl+0x3f1/0x3a30 drivers/tty/vt/vt_ioctl.c:364

This is ioctl(TIOCL_PASTESEL).
Locks held on the path: sel_lock -> termios_rwsem

> other info that might help us debug this:
>
> Chain exists of:
>   &tty->termios_rwsem --> console_lock --> sel_lock

Clearly. From the above, we have:
 console_lock -> sel_lock
 sel_lock -> termios_rwsem
 termios_rwsem -> console_lock

Fix this by reversing the console_lock -> sel_lock dependency in
ioctl(TIOCL_SETSEL). First, lock sel_lock, then console_lock.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Reported-by: syzbot+26183d9746e62da329b8@syzkaller.appspotmail.com
Fixes: 07e6124a1a46 ("vt: selection, close sel_buffer race")
---
 drivers/tty/vt/selection.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 5b0b897396eb..7556139cd0da 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -219,7 +219,6 @@ static int __set_selection_kernel(struct tiocl_selection *v, struct tty_struct *
 	if (ps > pe)	/* make sel_start <= sel_end */
 		swap(ps, pe);
 
-	mutex_lock(&sel_lock);
 	if (sel_cons != vc_cons[fg_console].d) {
 		clear_selection();
 		sel_cons = vc_cons[fg_console].d;
@@ -265,10 +264,9 @@ static int __set_selection_kernel(struct tiocl_selection *v, struct tty_struct *
 			break;
 		case TIOCL_SELPOINTER:
 			highlight_pointer(pe);
-			goto unlock;
+			return 0;
 		default:
-			ret = -EINVAL;
-			goto unlock;
+			return -EINVAL;
 	}
 
 	/* remove the pointer */
@@ -290,7 +288,7 @@ static int __set_selection_kernel(struct tiocl_selection *v, struct tty_struct *
 	else if (new_sel_start == sel_start)
 	{
 		if (new_sel_end == sel_end)	/* no action required */
-			goto unlock;
+			return 0;
 		else if (new_sel_end > sel_end)	/* extend to right */
 			highlight(sel_end + 2, new_sel_end);
 		else				/* contract from right */
@@ -318,8 +316,7 @@ static int __set_selection_kernel(struct tiocl_selection *v, struct tty_struct *
 	if (!bp) {
 		printk(KERN_WARNING "selection: kmalloc() failed\n");
 		clear_selection();
-		ret = -ENOMEM;
-		goto unlock;
+		return -ENOMEM;
 	}
 	kfree(sel_buffer);
 	sel_buffer = bp;
@@ -344,8 +341,7 @@ static int __set_selection_kernel(struct tiocl_selection *v, struct tty_struct *
 		}
 	}
 	sel_buffer_lth = bp - sel_buffer;
-unlock:
-	mutex_unlock(&sel_lock);
+
 	return ret;
 }
 
@@ -353,9 +349,11 @@ int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
 {
 	int ret;
 
+	mutex_lock(&sel_lock);
 	console_lock();
 	ret = __set_selection_kernel(v, tty);
 	console_unlock();
+	mutex_unlock(&sel_lock);
 
 	return ret;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] vt: selection, push sel_lock up
  2020-02-28 11:54 ` [PATCH 2/2] vt: selection, push sel_lock up Jiri Slaby
@ 2020-02-28 12:03   ` Greg KH
  2020-02-28 12:59     ` Jiri Slaby
  0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2020-02-28 12:03 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: linux-serial, linux-kernel, syzbot+26183d9746e62da329b8

On Fri, Feb 28, 2020 at 12:54:06PM +0100, Jiri Slaby wrote:
> sel_lock cannot nest in the console lock. Thanks to syzkaller, the
> kernel states firmly:
> 
> > WARNING: possible circular locking dependency detected
> > 5.6.0-rc3-syzkaller #0 Not tainted
> > ------------------------------------------------------
> > syz-executor.4/20336 is trying to acquire lock:
> > ffff8880a2e952a0 (&tty->termios_rwsem){++++}, at: tty_unthrottle+0x22/0x100 drivers/tty/tty_ioctl.c:136
> >
> > but task is already holding lock:
> > ffffffff89462e70 (sel_lock){+.+.}, at: paste_selection+0x118/0x470 drivers/tty/vt/selection.c:374
> >
> > which lock already depends on the new lock.
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #2 (sel_lock){+.+.}:
> >        mutex_lock_nested+0x1b/0x30 kernel/locking/mutex.c:1118
> >        set_selection_kernel+0x3b8/0x18a0 drivers/tty/vt/selection.c:217
> >        set_selection_user+0x63/0x80 drivers/tty/vt/selection.c:181
> >        tioclinux+0x103/0x530 drivers/tty/vt/vt.c:3050
> >        vt_ioctl+0x3f1/0x3a30 drivers/tty/vt/vt_ioctl.c:364
> 
> This is ioctl(TIOCL_SETSEL).
> Locks held on the path: console_lock -> sel_lock
> 
> > -> #1 (console_lock){+.+.}:
> >        console_lock+0x46/0x70 kernel/printk/printk.c:2289
> >        con_flush_chars+0x50/0x650 drivers/tty/vt/vt.c:3223
> >        n_tty_write+0xeae/0x1200 drivers/tty/n_tty.c:2350
> >        do_tty_write drivers/tty/tty_io.c:962 [inline]
> >        tty_write+0x5a1/0x950 drivers/tty/tty_io.c:1046
> 
> This is write().
> Locks held on the path: termios_rwsem -> console_lock
> 
> > -> #0 (&tty->termios_rwsem){++++}:
> >        down_write+0x57/0x140 kernel/locking/rwsem.c:1534
> >        tty_unthrottle+0x22/0x100 drivers/tty/tty_ioctl.c:136
> >        mkiss_receive_buf+0x12aa/0x1340 drivers/net/hamradio/mkiss.c:902
> >        tty_ldisc_receive_buf+0x12f/0x170 drivers/tty/tty_buffer.c:465
> >        paste_selection+0x346/0x470 drivers/tty/vt/selection.c:389
> >        tioclinux+0x121/0x530 drivers/tty/vt/vt.c:3055
> >        vt_ioctl+0x3f1/0x3a30 drivers/tty/vt/vt_ioctl.c:364
> 
> This is ioctl(TIOCL_PASTESEL).
> Locks held on the path: sel_lock -> termios_rwsem
> 
> > other info that might help us debug this:
> >
> > Chain exists of:
> >   &tty->termios_rwsem --> console_lock --> sel_lock
> 
> Clearly. From the above, we have:
>  console_lock -> sel_lock
>  sel_lock -> termios_rwsem
>  termios_rwsem -> console_lock
> 
> Fix this by reversing the console_lock -> sel_lock dependency in
> ioctl(TIOCL_SETSEL). First, lock sel_lock, then console_lock.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Reported-by: syzbot+26183d9746e62da329b8@syzkaller.appspotmail.com
> Fixes: 07e6124a1a46 ("vt: selection, close sel_buffer race")

As 07e6124a1a46 was marked for stable, both of these should be as well,
right?

And did you happen to test these two with the syzbot tool to see if it
really did fix the report?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] vt: selection, push sel_lock up
  2020-02-28 12:03   ` Greg KH
@ 2020-02-28 12:59     ` Jiri Slaby
  2020-02-28 13:04       ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: Jiri Slaby @ 2020-02-28 12:59 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-serial, linux-kernel, syzbot+26183d9746e62da329b8

On 28. 02. 20, 13:03, Greg KH wrote:
> On Fri, Feb 28, 2020 at 12:54:06PM +0100, Jiri Slaby wrote:
>> sel_lock cannot nest in the console lock. Thanks to syzkaller, the
>> kernel states firmly:
>>
>>> WARNING: possible circular locking dependency detected
>>> 5.6.0-rc3-syzkaller #0 Not tainted
>>> ------------------------------------------------------
>>> syz-executor.4/20336 is trying to acquire lock:
>>> ffff8880a2e952a0 (&tty->termios_rwsem){++++}, at: tty_unthrottle+0x22/0x100 drivers/tty/tty_ioctl.c:136
...
>>> other info that might help us debug this:
>>>
>>> Chain exists of:
>>>   &tty->termios_rwsem --> console_lock --> sel_lock
>>
>> Clearly. From the above, we have:
>>  console_lock -> sel_lock
>>  sel_lock -> termios_rwsem
>>  termios_rwsem -> console_lock
>>
>> Fix this by reversing the console_lock -> sel_lock dependency in
>> ioctl(TIOCL_SETSEL). First, lock sel_lock, then console_lock.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Reported-by: syzbot+26183d9746e62da329b8@syzkaller.appspotmail.com
>> Fixes: 07e6124a1a46 ("vt: selection, close sel_buffer race")
> 
> As 07e6124a1a46 was marked for stable, both of these should be as well,
> right?

Ah, yes. My bad again, sorry.

> And did you happen to test these two with the syzbot tool to see if it
> really did fix the report?

Nope, this syz* stuff is a black magic for me. How can I do that?

thanks,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] vt: selection, push sel_lock up
  2020-02-28 12:59     ` Jiri Slaby
@ 2020-02-28 13:04       ` Greg KH
  0 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2020-02-28 13:04 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: linux-serial, linux-kernel, syzbot+26183d9746e62da329b8

On Fri, Feb 28, 2020 at 01:59:36PM +0100, Jiri Slaby wrote:
> On 28. 02. 20, 13:03, Greg KH wrote:
> > On Fri, Feb 28, 2020 at 12:54:06PM +0100, Jiri Slaby wrote:
> >> sel_lock cannot nest in the console lock. Thanks to syzkaller, the
> >> kernel states firmly:
> >>
> >>> WARNING: possible circular locking dependency detected
> >>> 5.6.0-rc3-syzkaller #0 Not tainted
> >>> ------------------------------------------------------
> >>> syz-executor.4/20336 is trying to acquire lock:
> >>> ffff8880a2e952a0 (&tty->termios_rwsem){++++}, at: tty_unthrottle+0x22/0x100 drivers/tty/tty_ioctl.c:136
> ...
> >>> other info that might help us debug this:
> >>>
> >>> Chain exists of:
> >>>   &tty->termios_rwsem --> console_lock --> sel_lock
> >>
> >> Clearly. From the above, we have:
> >>  console_lock -> sel_lock
> >>  sel_lock -> termios_rwsem
> >>  termios_rwsem -> console_lock
> >>
> >> Fix this by reversing the console_lock -> sel_lock dependency in
> >> ioctl(TIOCL_SETSEL). First, lock sel_lock, then console_lock.
> >>
> >> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> >> Reported-by: syzbot+26183d9746e62da329b8@syzkaller.appspotmail.com
> >> Fixes: 07e6124a1a46 ("vt: selection, close sel_buffer race")
> > 
> > As 07e6124a1a46 was marked for stable, both of these should be as well,
> > right?
> 
> Ah, yes. My bad again, sorry.
> 
> > And did you happen to test these two with the syzbot tool to see if it
> > really did fix the report?
> 
> Nope, this syz* stuff is a black magic for me. How can I do that?

From the syzbot report at the bottom it says:
	syzbot will keep track of this bug report. See:
	https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
	syzbot can test patches for this bug, for details see:
	https://goo.gl/tpsmEJ#testing-patches

Try running these through that and let's see if we get a "success"
report or not.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-28 13:04 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-28 11:54 [PATCH 1/2] vt: selection, push console lock down Jiri Slaby
2020-02-28 11:54 ` [PATCH 2/2] vt: selection, push sel_lock up Jiri Slaby
2020-02-28 12:03   ` Greg KH
2020-02-28 12:59     ` Jiri Slaby
2020-02-28 13:04       ` Greg KH

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.