On Wed, Jan 1, 2014 at 8:49 PM, Tetsuo Handa
<penguin-ker...@i-love.sakura.ne.jp> wrote:
> From: Tetsuo Handa <penguin-ker...@i-love.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 <a...@linux-foundation.org>
> Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
