linux-kernel - Re: [PATCH v3 3/3] vsprintf: Move space out of string literals in fourcc

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: <YfJ8IhqsNtyfrsfO@paasikivi.fi.intel.com>
Date:   Thu, 27 Jan 2022 13:04:02 +0200
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Francis Laniel <laniel_francis@...vacyrequired.com>,
        Petr Mladek <pmladek@...e.com>, linux-kernel@...r.kernel.org,
        Andy Shevchenko <andy@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH v3 3/3] vsprintf: Move space out of string literals in
 fourcc_string()

Hi Kees,

On Wed, Jan 26, 2022 at 01:22:32PM -0800, Kees Cook wrote:
> On Wed, Jan 26, 2022 at 04:19:17PM +0200, Andy Shevchenko wrote:
> > The literals "big-endian" and "little-endian" may be potentially
> > occurred in other places. Dropping space allows linker to
> > "compress" them by using only a single copy.
> > 
> > Rasmus suggested, while at it, replacing strcpy() + strlen() by
> > p = stpcpy(), which is done here as well.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > Acked-by: Sakari Ailus <sakari.ailus@...ux.intel.com>
> > ---
> > v3: no changes
> >  lib/vsprintf.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 4e8f3e9acb99..e2a1d89f1a5c 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1781,8 +1781,8 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> >  		*p++ = isascii(c) && isprint(c) ? c : '.';
> >  	}
> >  
> > -	strcpy(p, orig & BIT(31) ? " big-endian" : " little-endian");
> > -	p += strlen(p);
> > +	*p++ = ' ';
> > +	p = stpcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> >  
> >  	*p++ = ' ';
> >  	*p++ = '(';
> 
> No need for any of the manual construction nor stpcpy():
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7ad44f2c8f5..aef8bd2789a9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1788,14 +1788,9 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
>  		*p++ = isascii(c) && isprint(c) ? c : '.';
>  	}
>  
> -	strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
> -	p += strlen(p);
> -
> -	*p++ = ' ';
> -	*p++ = '(';
> -	p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, sizeof(u32));
> -	*p++ = ')';
> -	*p = '\0';
> +	snprintf(p, sizeof(output) - sizeof(*fourcc), " %s (0x%x)",
> +		*fourcc & BIT(31) ? "big-endian" : " little-endian",
> +		*fourcc);
>  
>  	return string(buf, end, output, spec);
>  }
> 
> 
> (untested)

This could work but results in two nested calls to vsnprintf(). I'd
definitely avoid that, due to stack usage and then albeit it's easy to see
the recursion will end with two iterations, that will very fast cease to be
the case if you keep applying the pattern.

-- 
Regards,

Sakari Ailus