linux-kernel - Re: [PATCH] vt: Fix non-blinking cursor regression

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200126124834.ea7uxyprz2uaek5x@wunner.de>
Date:   Sun, 26 Jan 2020 13:48:34 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     Nicolas Pitre <nico@...xnic.net>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Mikulas Patocka <mpatocka@...hat.com>,
        Matthew Whitehead <tedheadster@...il.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Arvind Sankar <nivedita@...m.mit.edu>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vt: Fix non-blinking cursor regression

On Wed, Jan 22, 2020 at 11:40:38AM -0500, Nicolas Pitre wrote:
> On Wed, 22 Jan 2020, Lukas Wunner wrote:
> > Since commit a6dbe4427559 ("vt: perform safe console erase in the right
> > order"), when userspace clears both the scrollback buffer and the screen
> > by writing "\e[3J" to an fbdev virtual console, the cursor stops blinking
> > if that virtual console is not in the foreground.  I'm witnessing this
> > on every boot of Raspbian since updating to v4.19.37+ because agetty
> > writes the sequence to /dev/tty6 while the console is still switched to
> > /dev/tty1.  Switching consoles once makes the cursor blink again.
> > 
> > The commit added an invocation of ->con_switch() to flush_scrollback().
> > Normally this is only invoked from switch_screen() to switch consoles.
> > switch_screen() updates *vc->vc_display_fg to the new console and
> > fbcon_switch() updates ops->currcon.  Because the commit only invokes
> > fbcon_switch() but doesn't update *vc->vc_display_fg, it performs an
> > incomplete console switch.
> > 
> > When fb_flashcursor() subsequently blinks the cursor, it retrieves the
> > foreground console from ops->currcon.  Because *vc->vc_display_fg wasn't
> > updated, con_is_visible() incorrectly returns false and as a result,
> > fb_flashcursor() bails out without blinking the cursor.
> > 
> > The invocation of ->con_switch() appears to have been erroneous.  After
> > all, why should a console switch be performed when clearing the screen?
> > The commit message doesn't provide a rationale either.  So delete it.
> 
> The problem here is that only vgacon provides a con_flush_scrollback 
> method. When not provided, the only way to flush the scrollback buffer 
> is to invoke the switch method. If you remove it the scrollback buffer 
> of the foreground console won't be flushed in the fb case and possibly 
> others.

Okay.  I guess it's somewhat counter-intuitive that ->con_switch()
is called only because it has the side effect of flushing scrollback.
In particular, this approach doesn't work for nonvisible consoles.

So the proper solution might be to amend the fb_con struct with a
->con_flush_scrollback() hook.  Which portions of fbcon_switch()
would have to be duplicated in that hook?  The softback code at the
top of the function would seem like an obvious candidate.  What about
the invocation of fb_set_var() (which in turn calls fb_pan_display())?
Anything else?

FWIW, even without any call to ->con_switch, writing "\e[3J" does
flush scrollback.  Works both on the foreground console as well as
nonvisible consoles.  I've tested this with bcm2708_fb which isn't
upstream yet, it's in the Raspberry Pi Foundation's downstream tree:

https://github.com/raspberrypi/linux/blob/rpi-4.19.y/drivers/video/fbdev/bcm2708_fb.c


> @@ -936,10 +936,13 @@ static void flush_scrollback(struct vc_data *vc)
>  	WARN_CONSOLE_UNLOCKED();
>  
>  	set_origin(vc);
> -	if (vc->vc_sw->con_flush_scrollback)
> +	if (vc->vc_sw->con_flush_scrollback) {
>  		vc->vc_sw->con_flush_scrollback(vc);
> -	else
> +	} else if (con_is_visible(vc)) {
> +		hide_cursor(vc);
>  		vc->vc_sw->con_switch(vc);
> +		set_cursor(vc);
> +	}

A dumb question perhaps, but why is it necessary to hide the cursor?

Thanks,

Lukas