linux-kernel - Re: [PATCH v2 4/4] vt: coherence validation code for the unicode screen buffer

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]

Date:   Mon, 18 Jun 2018 21:50:08 -0400 (EDT)
From:   Nicolas Pitre <nicolas.pitre@...aro.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Dave Mielke <Dave@...lke.cc>,
        Samuel Thibault <samuel.thibault@...-lyon.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/4] vt: coherence validation code for the unicode
 screen buffer

On Tue, 19 Jun 2018, Andy Shevchenko wrote:

> On Sun, Jun 17, 2018 at 10:07 PM, Nicolas Pitre
> <nicolas.pitre@...aro.org> wrote:
> > Make sure the unicode screen buffer matches the video screen content.
> > This is provided for debugging convenience and disabled by default.
> 
> > +#define VC_UNI_SCREEN_DEBUG 0
> 
> > +       if (!VC_UNI_SCREEN_DEBUG || !uniscr)
> > +               return;
> 
> Hmm... Interesting. I would rather go with
> #ifdef ..._DEBUG
> ...
> #else
> return;
> #endif
> 
> It will relax requirement on how to define _DEBUG. I don't recall I
> see something like you proposing in the kernel for the same purpose.

Some random examples:

include/crypto/scatterwalk.h:106;
                if (ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE && !PageSlab(page))

include/math-emu/single.h:68:
    if (!FP_INHIBIT_RESULTS)

This form also allows for the compiler to parse and validate the code 
whether or not the feature is enabled, while still optimizing it away in 
the end if not enabled.

> > +
> > +       WARN_CONSOLE_UNLOCKED();
> > +
> > +       /*
> > +        * Make sure our unicode screen translates into the same glyphs
> > +        * as the actual screen. This is brutal indeed.
> > +        */
> > +       p = (unsigned short *)vc->vc_origin;
> > +       mask = vc->vc_hi_font_mask | 0xff;
> > +       for (y = 0; y < vc->vc_rows; y++) {
> > +               char32_t *line = uniscr->lines[y];
> > +               for (x = 0; x < vc->vc_cols; x++) {
> > +                       u16 glyph = scr_readw(p++) & mask;
> > +                       char32_t uc = line[x];
> > +                       int tc = conv_uni_to_pc(vc, uc);
> > +                       if (tc == -4)
> > +                               tc = conv_uni_to_pc(vc, 0xfffd);
> > +                       if (tc == -4)
> > +                               tc = conv_uni_to_pc(vc, '?');
> > +                       if (tc != glyph)
> 
> > +                               pr_notice("%s: mismatch at %d,%d: "
> > +                                         "glyph=%#x tc=%#x\n", __func__,
> > +                                         x, y, glyph, tc);
> 
> Don't split format string in printk(). checkpatch will not warn on longer lines.

I didn't do it like that for checkpatch but to keep the code readable.
I don't particularly care either ways though.


Nicolas