linux-kernel - Re: [RFC PATCH] vsprintf: add flag ZEROPAD handling before crng is ready

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: <1516958276.7000.1271.camel@linux.intel.com>
Date:   Fri, 26 Jan 2018 11:17:56 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Yang Shunyong <shunyong.yang@...-semitech.com>, me@...in.cc,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     akpm@...ux-foundation.org, joe@...ches.com,
        pantelis.antoniou@...sulko.com, mchehab@...nel.org,
        adobriyan@...il.com, linux-kernel@...r.kernel.org,
        yu.zheng@...-semitech.com
Subject: Re: [RFC PATCH] vsprintf: add flag ZEROPAD handling before crng is
 ready

+Rasmus

On Fri, 2018-01-26 at 15:39 +0800, Yang Shunyong wrote:
> Before crng is ready, output of "%p" composes of "(ptrval)" and
> left padding spaces for alignment as no random address can be
> generated. This seems a little strange sometimes.
> For example, when irq domain names are built with "%p", the nodes
> under /sys/kernel/debug/irq/domains like this,
> 
> [root@y irq]# ls domains/
> default                   irqchip@        (ptrval)-2
> irqchip@        (ptrval)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
> irqchip@        (ptrval)  irqchip@        (ptrval)-3
> \_SB_.TCS0.QIC0             \_SB_.TCS0.QIC2
> 
> The name "irqchip@        (ptrval)-2" is not so readable in console
> output.

Yes, this is not best output.

> This patch adds ZEROPAD handling in widen_string() and move_right().
> When ZEROPAD is set in spec, it will use '0' for padding. If not
> set, it will use ' '.
> This patch also sets ZEROPAD in ptr_to_id() before crgn is ready.

Did you run tests?

Have you added specific test cases to see what's going on for patterns
like

printf("%0s\n", "    (my string)");

?

> Following is the output after applying the patch,
> 
> [root@y irq]# ls domains/
> default                   irqchip@...00000(ptrval)-2
> irqchip@...00000(ptrval)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
> irqchip@...00000(ptrval)  irqchip@...00000(ptrval)-3  \_SB_.TCS0.QIC0
> \_SB_.TCS0.QIC2
> 

So, for me it looks like curing symptoms. After all, it's debugfs, no
one prevents you to fix an output of the certain nodes there.

> I am not sure whether crng can get enough random data at very early
> stage of system startup (eg. before irq system in the case above)
> and the effort to port current random driver to work at that time.
> So, this issue may exist some time.
> I use '0' for padding in this patch. This should be ok because output
> of "%pK" is all '0's when kptr_restrict = 2. I don't know whether
> other character, such as 'x', may be more preferable.
> 
> Signed-off-by: Yang Shunyong <shunyong.yang@...-semitech.com>
> ---
>  lib/vsprintf.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 01c3957b2de6..e0b6e1edae31 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -535,14 +535,18 @@ char *special_hex_number(char *buf, char *end,
> unsigned long long num, int size)
>  	return number(buf, end, num, spec);
>  }
>  
> -static void move_right(char *buf, char *end, unsigned len, unsigned
> spaces)
> +static void move_right(char *buf, char *end, unsigned int len,
> +		       unsigned int spaces, struct printf_spec spec)
>  {
>  	size_t size;
> +	char pad;
> +
> +	pad = (spec.flags & ZEROPAD) ? '0' : ' ';
>  	if (buf >= end)	/* nowhere to put anything */
>  		return;
>  	size = end - buf;
>  	if (size <= spaces) {
> -		memset(buf, ' ', size);
> +		memset(buf, pad, size);
>  		return;
>  	}
>  	if (len) {
> @@ -550,7 +554,7 @@ static void move_right(char *buf, char *end,
> unsigned len, unsigned spaces)
>  			len = size - spaces;
>  		memmove(buf + spaces, buf, len);
>  	}
> -	memset(buf, ' ', spaces);
> +	memset(buf, pad, spaces);
>  }
>  
>  /*
> @@ -565,18 +569,21 @@ static void move_right(char *buf, char *end,
> unsigned len, unsigned spaces)
>  char *widen_string(char *buf, int n, char *end, struct printf_spec
> spec)
>  {
>  	unsigned spaces;
> +	char pad;
>  
>  	if (likely(n >= spec.field_width))
>  		return buf;
>  	/* we want to pad the sucker */
>  	spaces = spec.field_width - n;
>  	if (!(spec.flags & LEFT)) {
> -		move_right(buf - n, end, n, spaces);
> +		move_right(buf - n, end, n, spaces, spec);
>  		return buf + spaces;
>  	}
> +
> +	pad = (spec.flags & ZEROPAD) ? '0' : ' ';
>  	while (spaces--) {
>  		if (buf < end)
> -			*buf = ' ';
> +			*buf = pad;
>  		++buf;
>  	}
>  	return buf;
> @@ -1702,6 +1709,8 @@ static char *ptr_to_id(char *buf, char *end,
> void *ptr, struct printf_spec spec)
>  
>  	if (unlikely(!have_filled_random_ptr_key)) {
>  		spec.field_width = default_width;
> +		spec.flags |= ZEROPAD;
> +
>  		/* string length must be less than default_width */
>  		return string(buf, end, "(ptrval)", spec);
>  	}

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy