linux-kernel - Re: [PATCH v3] console: Add persistent scrollback buffers for all VGA consoles

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: <1479318019.20980.12.camel@gmx.de>
Date:   Wed, 16 Nov 2016 18:40:19 +0100
From:   Manuel Schölling <manuel.schoelling@....de>
To:     Andrey Utkin <andrey_utkin@...tmail.com>
Cc:     plagnioj@...osoft.com, tomi.valkeinen@...com, jslaby@...e.cz,
        gregkh@...uxfoundation.org, linux-fbdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] console: Add persistent scrollback buffers for all
 VGA consoles

Hi,

I finally found the time to rework the patches, but there are a few
things I want to discuss:

> > This breaks tools like clear_console that rely on flushing the
> > scrollback history by switching back and forth between consoles
> > which is why this feature is disabled by default.
> > Use the escape sequence \e[3J instead for flushing the buffer.
> 
> Have never heard of such tool as clear_console. Why mention it at all,
> where is it used, which are practical configurations which are broken in
> this way?
clear_console(1) is part of Debian's bash package and (I think) a
inofficial 'downstream' patch developed by Ubuntu [1]. There has been
security concerns (which is also why I disabled this feature by
default).
However, if there might be security issues, I would strongly suggest to
keep this part in the documentation - it does no harm anyway, does it?

> Does this break clear(1)?
No, my tests with clear(1) worked fine.

> > -         Enter the amount of System RAM to allocate for the scrollback
> > -	 buffer.  Each 64KB will give you approximately 16 80x25
> > -	 screenfuls of scrollback buffer
> > +	  Enter the amount of System RAM to allocate for scrollback
> > +	  buffers of VGA consoles. Each 64KB will give you approximately
> > +	  16 80x25 screenfuls of scrollback buffer.
> 
> Indentation amended, I don't know which way it should be, please
> double-check or mimic old indentation to stay on safe side.
Double-checked! That's at least how I read the docs.

> > +static struct vgacon_scrollback_info *vgacon_scrollback_cur;
> 
> Could you use name vgacon_scrollback instead of new
> vgacon_scrollback_cur? It doesn't change anything conceptually, but is a
> bit shorter and seems to mean current scrollback context anyway, right?
> 
> I think I see nothing to comment on below.
There is a already an array called 'vgacon_scrollbacks[N]', so I would
not like to have two variables named 'vgacon_scrollback' and
'vgacon_scrollbacks'. Either {'vgacon_scrollback_cur' and
'vgacon_scrollbacks'} or {'vgacon_scrollback' and
'vgacon_scrollback_list'}.
You can make a suggestion, if you like.

All other point you mentioned where fixed as you suggested.

Thanks again for your help, Andrey!

Best,

Manuel

[1]
http://manpages.ubuntu.com/manpages/xenial/en/man1/clear_console.1.html