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:	Fri, 3 Jan 2014 09:08:25 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:	Joe Perches <joe@...ches.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Jiri Kosina <jkosina@...e.cz>,
	Al Viro <viro@...iv.linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] lib/vsprintf: add %pT[C012] format specifier

On Wed, Jan 1, 2014 at 8:49 PM, Tetsuo Handa
<penguin-kernel@...ove.sakura.ne.jp> wrote:
> From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Date: Thu, 2 Jan 2014 13:37:14 +0900
> Subject: [PATCH v3] lib/vsprintf: support built-in tokens
>
> The vast majority of ->comm accesses are accessing current->comm, for
> debug reasons. I'm counting 350-odd sites. At all these sites it's
> pointless passing `current' to the printk function at all!
>
> This patch introduces a set of vsprintf tokens for embedding globally visible
> arguments into the format string, so that we can reduce text size a bit (and
> in the future make reading of task_struct->comm consistent) by stop generating
> the code to pass an argument which the callee already has access to, with an
> assumption that currently we are not using the 0x1A byte within the format
> string.
>
> Some examples are shown below.
>
>   pr_info("comm=%s\n", current->comm);
>           => pr_info("comm=" EMBED_CURRENT_COMM "\n");
>
>   pr_info("pid=%d\n", current->pid);
>           => pr_info("pid=" EMBED_CURRENT_PID "\n");
>
>   pr_info("%s(%d)\n", current->comm, current->pid);
>           => pr_info(EMBED_CURRENT_COMM "(" EMBED_CURRENT_PID ")\n");
>
>   pr_info("[%-6.6s]\n", current->comm);
>           => pr_info(EMBED_FORMAT "-6.6" EMBED_CURRENT_COMM "\n");
>
> Since this patch does not use get_task_comm(), you should not convert from
> get_task_comm(current) to built-in tokens if you want constsistent reading.
>
> Suggested-by: Andrew Morton <akpm@...ux-foundation.org>
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> ---
>  include/linux/kernel.h |   17 ++++++++
>  lib/vsprintf.c         |  101 +++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 116 insertions(+), 2 deletions(-)
> [...]
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 10909c5..b05217d 100644
> @@ -1423,7 +1512,7 @@ int format_decode(const char *fmt, struct printf_spec *spec)
>         spec->type = FORMAT_TYPE_NONE;
>
>         for (; *fmt ; ++fmt) {
> -               if (*fmt == '%')
> +               if (*fmt == '%' || *fmt == 0x1A)
>                         break;
>         }
>

I'm on board with the idea of embedding comm/pid/whatever, but I
either missed or do not understand why a second format-start character
is being added. I think this will complicate audits and maybe trigger
weird info leaks (imagine printing a string that was %-escaped, but
not 0x1A-escaped?)

Why not use % followed by 0x1A to be the start code, instead of just 0x1A?

-Kees

-- 
Kees Cook
Chrome OS Security
--
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/