linux-kernel - Re: [PATCH v1] ipmi: use %*ph to print small buffer

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: <e0b24ff49eb69a216b11f97db1fc26c5d3b971b4.camel@perches.com>
Date:   Fri, 11 Oct 2019 08:18:41 -0700
From:   Joe Perches <joe@...ches.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Corey Minyard <minyard@....org>,
        openipmi-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] ipmi: use %*ph to print small buffer

On Fri, 2019-10-11 at 18:12 +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 07:58:14AM -0700, Joe Perches wrote:
> > On Fri, 2019-10-11 at 17:52 +0300, Andy Shevchenko wrote:
> > > Use %*ph format to print small buffer as hex string.
> > > 
> > > The change is safe since the specifier can handle up to 64 bytes and taking
> > > into account the buffer size of 100 bytes on stack the function has never been
> > > used to dump more than 32 bytes. Note, this also avoids potential buffer
> > > overflow if the length of the input buffer is bigger.
> > []
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > []
> > > @@ -48,14 +48,7 @@ static int handle_one_recv_msg(struct ipmi_smi *intf,
> > >  static void ipmi_debug_msg(const char *title, unsigned char *data,
> > >  			   unsigned int len)
> > >  {
> > > -	int i, pos;
> > > -	char buf[100];
> > > -
> > > -	pos = snprintf(buf, sizeof(buf), "%s: ", title);
> > > -	for (i = 0; i < len; i++)
> > > -		pos += snprintf(buf + pos, sizeof(buf) - pos,
> > > -				" %2.2x", data[i]);
> > > -	pr_debug("%s\n", buf);
> > > +	pr_debug("%s: %*ph\n", title, len, buf);
> > >  }
> > >  #else
> > >  static void ipmi_debug_msg(const char *title, unsigned char *data,
> > 
> > Now you might as well remove the #ifdef DEBUG above this
> > and the empty function in the #else too.
> 
> It's up to maintainer.

That's like suggesting any function with a single pr_debug
should have another duplicative empty function without.

Using code like the below is not good form as it's prone
to defects when the arguments in one block is changed but
not the other.

Also the first form doesn't work with dynamic debug.

#ifdef DEBUG
void debug_print(...)
{
	pr_debug(...);
}
#else
void debug_print(...)
{
}
#endif