Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 27 Jan 2022 13:04:02 +0200
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Kees Cook <keescook@chromium.org>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Francis Laniel <laniel_francis@privacyrequired.com>,
        Petr Mladek <pmladek@suse.com>, linux-kernel@vger.kernel.org,
        Andy Shevchenko <andy@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>
Subject: Re: [PATCH v3 3/3] vsprintf: Move space out of string literals in
 fourcc_string()
Message-ID: <YfJ8IhqsNtyfrsfO@paasikivi.fi.intel.com>
References: <20220126141917.75399-1-andriy.shevchenko@linux.intel.com>
 <20220126141917.75399-3-andriy.shevchenko@linux.intel.com>
 <202201261321.34794CCF3@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202201261321.34794CCF3@keescook>
Precedence: bulk

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@linux.intel.com>
> > Acked-by: Sakari Ailus <sakari.ailus@linux.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