linux-kernel - Re: [PATCH net-next v3 2/3] hexdump: Use for_each macro in print_hex

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] [day] [month] [year] [list]

Message-ID: <Z7elBmfBp0CGmgGh@li-4c4c4544-0047-5210-804b-b8c04f323634.ibm.com>
Date: Thu, 20 Feb 2025 15:56:22 -0600
From: Nick Child <nnac123@...ux.ibm.com>
To: David Laight <david.laight.linux@...il.com>
Cc: Dave Marquardt <davemarq@...ux.ibm.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, horms@...nel.org, nick.child@....com,
        pmladek@...e.com, rostedt@...dmis.org, john.ogness@...utronix.de,
        senozhatsky@...omium.org
Subject: Re: [PATCH net-next v3 2/3] hexdump: Use for_each macro in
 print_hex_dump

On Thu, Feb 20, 2025 at 09:41:18PM +0000, David Laight wrote:
> On Thu, 20 Feb 2025 09:49:04 -0600
> Nick Child <nnac123@...ux.ibm.com> wrote:
> > On 2/19/25 3:54 PM, Dave Marquardt wrote:
> > > Nick Child <nnac123@...ux.ibm.com> writes:
> > >  
> > >> diff --git a/lib/hexdump.c b/lib/hexdump.c
> > >> index c3db7c3a7643..181b82dfe40d 100644
> > >> --- a/lib/hexdump.c
> > >> +++ b/lib/hexdump.c
> > >> @@ -263,19 +263,14 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> > >>   		    const void *buf, size_t len, bool ascii)
> > >>   {
> > >> -	for (i = 0; i < len; i += rowsize) {
> > >> -		linelen = min(remaining, rowsize);
> > >> -		remaining -= rowsize;
> > >> -
> > >> -		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> > >> -				   linebuf, sizeof(linebuf), ascii);
> > >> -
> > >> +	for_each_line_in_hex_dump(i, rowsize, linebuf, sizeof(linebuf),
> > >> +				  groupsize, buf, len) {  
> > > Several callers of print_hex_dump pass true for the ascii parameter,
> > > which gets passed along to hex_dump_to_buffer. But you ignore it in
> > > for_each_line_in_hex_dump and always use false:
> > >
> > > + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> > > +				   buf, len) \
> > > +	for ((i) = 0;							\
> > > +	     (i) < (len) &&						\
> > > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > > +				(len) - (i), (rowsize), (groupsize),	\
> > > +				(linebuf), (linebuflen), false);	\
> > > +	     (i) += (rowsize) == 32 ? 32 : 16				\
> > > +	    )
> > >
> > > Is this behavior change intended?
> > >
> > > -Dave  
> > 
> > Yes, for simplicity, I wanted to limit the number of parameters that the 
> > macro had.
> > 
> > Since the function does not do any printing, the user can do ascii 
> > conversion on their own
> > 
> > or even easier just ensure a \NULL term and print the string with the %s 
> > format specifier.
> 
> That just isn't the same.
> The hexdump code 'sanitises' the string for you.
> 

I see now, I forgot about print_hex_dump's use of
for_each_line_in_hexdump in my response the morning. Yes the
ascii argument needs to be respected and this patch forces
false, this is an oversight on my end, thanks for pointing it out.

I will address this issue in the next version by passing the ascii
argument into the for_each loop. Will wait a few weeks before sending
this time.

- Nick