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]

Date:	Wed, 8 Jan 2014 15:46:25 +0100
From:	Pavel Machek <pavel@....cz>
To:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:	joe@...ches.com, 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

On Wed 2014-01-08 23:19:04, Tetsuo Handa wrote:
> 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?

Compilers.

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

I like this one.

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

I think this is not a good idea.

> 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.

Agreed. No patches make compilers unhappy. I thought ("%pT", NULL) is
done that way not to confuse compilers. ("%pT") would trigger checks,
right?

> 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

Yeah, but I still think magically replacing \x1A is unexpected
behaviour.

> It seems to me that we can agree with the "%pT" patch.

Yes.

> But how does the "\x1A" patch make compilers or kernel developers unhappy?

It makes me unhappy -- one more character to escape, and unexpected
one. (With possible security implications -- think sprintf(buf, "string
without %s, from user").

But in this thread, I was arguing that %pT is a good idea, should be
applied, and no more bike shedding is neccessary.

(Feel free to add  Acked-by:/Reviewed-by: Pavel Machek <pavel@....cz>
if you wish.)

Thanks,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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/