linux-kernel - Re: [PATCH] lib/vsprintf: add %pT[C012] format specifier

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: <201401082319.FEC82827.JFFMOQSHOOVtFL@I-love.SAKURA.ne.jp>
Date:	Wed, 8 Jan 2014 23:19:04 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	pavel@....cz, joe@...ches.com
Cc:	keescook@...omium.org, akpm@...ux-foundation.org,
	geert@...ux-m68k.org, jkosina@...e.cz, viro@...iv.linux.org.uk,
	davem@...emloft.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lib/vsprintf: add %pT[C012] format specifier

Pavel Machek wrote:
> > > > I'm not nacking this, just stating my view.
> > > 
> > > And I believe Andrew clearly stated his view, on the very topic you
> > > asked him on.
> > 
> > I believe Andrew's view:
> > 
> > On Sat, 2013-12-28 at 12:08 -0800, Andrew Morton wrote:
> > > On Sat, 28 Dec 2013 11:53:25 -0800 Joe Perches <joe@...ches.com> wrote:
> > > > Tell me again, what's wrong with using p or current?
> > > > printk("%pt", current);
> > > 
> > > Nothing much.  It's just that all these callsites are generating the
> > > code to pass an argument which the callee already has access to. 
> > > Optimizing that will reduce text size a bit.
> > 
> > was that the argument passing was the primary issue.
> 
> Yes. He dislikes passing argument callee has already access
> to. "current". This patch does not do this, it just passes the NULL as
> a marker... and to keep printf() checkers happy.

Excuse me?
Compilers or kernel developers, which one does "printf() checkers" refer to?

The "%pT" patch is for printing tsk->comm consistently, but not always
tsk == current .

The "\x1A" patch is for printing tsk->xxx without passing tsk->xxx as
arguments, but always tsk == current .

These two patches have different purposes and do not overwrap.

Both patches are written not to confuse compilers when using
__attribute__((format(printf, a, b))) check. Therefore, I don't think that
the "\x1A" patch makes compilers unhappy. Rather, the "\x1A" patch would make
compilers more happy because it replaces cost of passing current or NULL
as arguments with cost of symbolic names (e.g. two bytes for
  #define EMBED_CURRENT_COMM "\x1A\xFF"
case) within the format string.

  (no patch applied)
  tsk = current;
  pr_info("%s[%d] trap %s ip:%lx sp:%lx error:%lx",
          tsk->comm, tsk->pid, str, regs->ip, regs->sp, error_code);

  (the "%pT" patch applied)
  tsk = current;
  pr_info("%pT[%d] trap %s ip:%lx sp:%lx error:%lx",
          NULL, tsk->pid, str, regs->ip, regs->sp, error_code);

  (the "\x1A" patch applied)
  pr_info(EMBED_CURRENT_COMM "[" EMBED_CURRENT_PID "] trap %s ip:%lx sp:%lx error:%lx",
          str, regs->ip, regs->sp, error_code);

By reducing number of function arguments, those who audit the correctness of
kernel source code would gain more readability.

  (no patch applied)
  tsk = current;
  pr_info("%s[%d] trap %s ip:%lx sp:%lx error:%lx",
          tsk->comm, tsk->pid, str, regs->ip, regs->sp, error_code);

  (the "\x1A" patch applied)
  pr_info(EMBED_CURRENT_COMM "[" EMBED_CURRENT_PID "] trap %s ip:%lx sp:%lx error:%lx",
          str, regs->ip, regs->sp, error_code);

There might be kernel developers who are using \x1A within the format string as
a literal byte, but we will be able to find and rewrite \x1A in the format
string like

  (before fixing problem)
  const chat *fmt = "aaa\x1Axxx\n";
  pr_debug(fmt);

  (after fixing problem)
  const char *str = "aaa\x1Axxx"
  pr_debug("%s\n", str);

as with we find and rewrite '%' in the format string like

  (before fixing problem)
  const chat *fmt = "aaa%xxx\n";
  pr_debug(fmt);

  (after fixing problem)
  const char *str = "aaa%xxx"
  pr_debug("%s\n", str);

.

It seems to me that we can agree with the "%pT" patch.
But how does the "\x1A" patch make compilers or kernel developers unhappy?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/