linux-kernel - Re: [PATCH v4] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

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: <Z8GpJDGeJVHbIy8X@smile.fi.intel.com>
Date: Fri, 28 Feb 2025 14:16:36 +0200
From: "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>
To: Aditya Garg <gargaditya08@...e.com>
Cc: "pmladek@...e.com" <pmladek@...e.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	"senozhatsky@...omium.org" <senozhatsky@...omium.org>,
	"corbet@....net" <corbet@....net>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"apw@...onical.com" <apw@...onical.com>,
	"joe@...ches.com" <joe@...ches.com>,
	"dwaipayanray1@...il.com" <dwaipayanray1@...il.com>,
	"lukas.bulwahn@...il.com" <lukas.bulwahn@...il.com>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Hector Martin <marcan@...can.st>,
	"sven@...npeter.dev" <sven@...npeter.dev>,
	Janne Grunau <j@...nau.net>,
	"alyssa@...enzweig.io" <alyssa@...enzweig.io>,
	"asahi@...ts.linux.dev" <asahi@...ts.linux.dev>
Subject: Re: [PATCH v4] lib/vsprintf: Add support for generic FOURCCs by
 extending %p4cc

On Thu, Feb 27, 2025 at 05:10:52PM +0000, Aditya Garg wrote:
> > On 27 Feb 2025, at 8:13 PM, andriy.shevchenko@...ux.intel.com wrote:
> > On Thu, Feb 27, 2025 at 06:30:48AM +0000, Aditya Garg wrote:

...

> >> +Generic FourCC code
> >> +-------------------
> >> +
> >> +::
> >> +    %p4c[hrbl]    gP00 (0x67503030)
> >> +
> >> +Print a generic FourCC code, as both ASCII characters and its numerical
> >> +value as hexadecimal.
> >> +
> >> +The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify
> >> +host, reversed, big or little endian order data respectively. Host endian
> >> +order means the data is interpreted as a 32-bit integer and the most
> >> +significant byte is printed first; that is, the character code as printed
> >> +matches the byte order stored in memory on big-endian systems, and is reversed
> >> +on little-endian systems.
> > 
> > Btw, this sounds to me that 'h' should be accompanied with 'n', otherwise it's
> > confusing why BE is the host order out of the blue.
> > so, it needs more information that this mimics htonl() / ntohl() for networking.
> > 
> > Does 'r' actually should be 'n'?
> 
> I believe you mean negative endian? Can be done.

No, 'network order' / 'host order'. That's where BE comes from, but you may ask
the original author about this. h/r pair makes little sense to me as it
inconsistent.

> >> +Passed by reference.
> >> +
> >> +Examples for a little-endian machine, given &(u32)0x67503030::
> >> +
> >> +    %p4ch    gP00 (0x67503030)
> >> +    %p4cr    00Pg (0x30305067)
> >> +    %p4cb    00Pg (0x30305067)
> >> +    %p4cl    gP00 (0x67503030)
> >> +
> >> +Examples for a big-endian machine, given &(u32)0x67503030::
> >> +
> >> +    %p4ch    gP00 (0x67503030)
> >> +    %p4cr    00Pg (0x30305067)
> >> +    %p4cb    gP00 (0x67503030)
> >> +    %p4cl    00Pg (0x30305067)
> >> +

...

> >> +    switch (fmt[2]) {
> >> +    case 'h':
> >> +        val = orig;
> >> +        break;
> >> +    case 'r':
> >> +        orig = swab32(orig);
> >> +        val = orig;
> >> +        break;
> >> +    case 'l':
> >> +        orig = (__force u32)cpu_to_le32(orig);
> >> +        val = orig;
> >> +        break;
> >> +    case 'b':
> >> +        orig = (__force u32)cpu_to_be32(orig);
> >> +        val = orig;
> >> +        break;
> >> +    case 'c':
> >> +        /* Pixel formats are printed LSB-first */
> >> +        val = swab32(orig & ~BIT(31));
> >> +        pixel_fmt = true;
> >> +        break;
> >> +    default:
> >> +        return error_string(buf, end, "(%p4?)", spec);
> >> +    }
> > 
> > Actually you can replace all these orig copies by introducing a new boolean, pixel_be.
> > 
> > Will become
> > 
> >    switch (fmt[2]) {
> >    case 'h':
> >        val = orig;
> >        break;
> >    case 'r': // or 'n' ?
> >        val = swab32(orig);
> >        break;
> >    case 'l':
> >        val = (__force u32)cpu_to_le32(orig);
> >        break;
> >    case 'b':
> >        val = (__force u32)cpu_to_be32(orig);
> >        break;
> >    case 'c':
> >        pixel_fmt = true;
> >        pixel_be = orig & BIT(31);
> >        /* Pixel formats are printed LSB-first */
> >        val = swab32(orig & ~BIT(31));
> >        break;
> >    default:
> >        return error_string(buf, end, "(%p4?)", spec);
> >    }
> > 
> > And with this the existence of 'val' now becomes doubtful, we may reuse 'orig',
> > just name it 'val' everywhere, no?
> 
> In case c, val != orig, in rest it is. We can just use pixel_fmt to check
> this condition, but places where we use orig, and not val will need an if
> statement or something similar. Tbh, it's an unecessary complication. You
> might want to see this part of the code:

Fair enough.

> 	if (pixel_fmt) {
> 		*p++ = ' ';
> 		strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> 		p += strlen(p);
> 	}
> 
> 	*p++ = ' ';
> 	*p++ = '(';
> 	p = special_hex_number(p, output + sizeof(output) - 2, orig, sizeof(u32));
> 	*p++ = ')';
> 	*p = '\0';
> 
> Here, special_hex_number is common to all cases.

I see, thanks for pointing this out.

-- 
With Best Regards,
Andy Shevchenko