| 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
| ||
|
Message-ID: <Z7xTE4jKsloFw2mq@smile.fi.intel.com>
Date: Mon, 24 Feb 2025 13:08:03 +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>,
"rostedt@...dmis.org" <rostedt@...dmis.org>,
"linux@...musvillemoes.dk" <linux@...musvillemoes.dk>,
"senozhatsky@...omium.org" <senozhatsky@...omium.org>,
"corbet@....net" <corbet@....net>,
"maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
"mripard@...nel.org" <mripard@...nel.org>,
"tzimmermann@...e.de" <tzimmermann@...e.de>,
"airlied@...il.com" <airlied@...il.com>,
"simona@...ll.ch" <simona@...ll.ch>,
"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>,
"sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
"christian.koenig@....com" <christian.koenig@....com>,
"kekrby@...il.com" <kekrby@...il.com>,
"admin@...eit.net" <admin@...eit.net>,
Orlando Chamberlain <orlandoch.dev@...il.com>,
"evepolonium@...il.com" <evepolonium@...il.com>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
Hector Martin <marcan@...can.st>,
"linux@...linux.org.uk" <linux@...linux.org.uk>,
"asahi@...ts.linux.dev" <asahi@...ts.linux.dev>,
Sven Peter <sven@...npeter.dev>, Janne Grunau <j@...nau.net>
Subject: Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by
extending %p4cc
On Sun, Feb 23, 2025 at 06:39:15AM +0000, Aditya Garg wrote:
> > On 22 Feb 2025, at 5:41 PM, Aditya Garg <gargaditya08@...e.com> wrote:
> >> On 21 Feb 2025, at 8:57 PM, andriy.shevchenko@...ux.intel.com wrote:
> >>> On Thu, Feb 20, 2025 at 04:39:23PM +0000, Aditya Garg wrote:
...
> >>> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> >>> it's useful to be able to print generic 4-character codes formatted as
> >>> an integer. Extend it to add format specifiers for printing generic
> >>> 32-bit FOURCCs with various endian semantics:
> >>> %p4ch Host-endian
> >>> %p4cl Little-endian
> >>> %p4cb Big-endian
> >>> %p4cr Reverse-endian
> >>> The endianness determines how bytes are interpreted as a u32, and the
> >>> FOURCC is then always printed MSByte-first (this is the opposite of
> >>> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> >>> allow printing LSByte-first FOURCCs stored in host endian order
> >>> (other than the hex form being in character order, not the integer
> >>> value).
> >> ...
> >>> orig = get_unaligned(fourcc);
> >>> - val = orig & ~BIT(31);
> >>> + switch (fmt[2]) {
> >>> + case 'h':
> >>> + val = orig;
> >>> + break;
> >>> + case 'r':
> >>> + orig = swab32(orig);
> >>> + val = orig;
> >>> + break;
> >>> + case 'l':
> >>> + orig = le32_to_cpu(orig);
> >>> + val = orig;
> >>> + break;
> >>> + case 'b':
> >>> + orig = be32_to_cpu(orig);
> >> I do not see that orig is a union of different types. Have you run sparse?
> >> It will definitely complain on this code.
> >
> > After messing around with this, what I’ve noticed is that orig and val used
> > in this struct should be u32. Now in case of little endian and big endian,
> > that things are messy. The original code by Hector was using le32_to_cpu on
> > orig, which itself is declared as a u32 here (maybe was done with the
> > intention to convert le32 orig to u32 orig?).
> >
> > Anyways, what I have done is that:
> >
> > 1. Declare new variable, orig_le which is __le32.
> > 2. Instead of doing orig = le32_to_cpu(orig); , we can do orig_le =
> > cpu_to_le32(orig). This fixes the sparse warning: cast to restricted __le32
> > 3. Now the original code was intending to use val=orig=le32_to_cpu(orig) at
> > the bottom part of this struct. Those parts also require val and orig to be
> > u32. For that, we are now using le32_to_cpu(orig_le). Since val is same as
> > orig, in case these cases, instead of making a val_le, I’ve simply used
> > orig_le there as well.
> >
> > Similar changes done for big endian.
> >
> > So, the struct looks like this now:
> >
> > static noinline_for_stack
> > char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > struct printf_spec spec, const char *fmt)
> > {
> > char output[sizeof("0123 little-endian (0x01234567)")];
> > char *p = output;
> > unsigned int i;
> > unsigned char c;
> > bool pixel_fmt = false;
> > u32 orig, val;
> > __le32 orig_le;
> > __be32 orig_be;
> >
> > if (fmt[1] != 'c')
> > return error_string(buf, end, "(%p4?)", spec);
> >
> > if (check_pointer(&buf, end, fourcc, spec))
> > return buf;
> >
> > orig = get_unaligned(fourcc);
> > switch (fmt[2]) {
> > case 'h':
> > val = orig;
> > break;
> > case 'r':
> > orig = swab32(orig);
> > val = orig;
> > break;
> > case 'l':
> > orig_le = cpu_to_le32(orig);
> > break;
> > case 'b':
> > orig_be = cpu_to_be32(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);
> > }
> >
> > for (i = 0; i < sizeof(u32); i++) {
> > switch (fmt[2]) {
> > case 'h':
> > case 'r':
> > case 'c':
> > c = val >> ((3 - i) * 8);
> > break;
> > case 'l':
> > c = le32_to_cpu(orig_le) >> ((3 - i) * 8);
> > break;
> > case 'b':
> > c = be32_to_cpu(orig_be) >> ((3 - i) * 8);
> > break;
This doesn't look right. It's basically two conversions from and to orig,
it's like using orig here, but that's wrong for the respective endianess,
'l'/BE should be LE, same for 'b'/LE which should be BE.
> > }
> >
> > /* Print non-control ASCII characters as-is, dot otherwise */
> > *p++ = isascii(c) && isprint(c) ? c : '.';
> > }
> >
> > if (pixel_fmt) {
> > *p++ = ' ';
> > strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> > p += strlen(p);
> > }
> >
> > *p++ = ' ';
> > *p++ = '(';
> >
> > switch (fmt[2]) {
> > case 'h':
> > case 'r':
> > case 'c':
> > p = special_hex_number(p, output + sizeof(output) - 2, orig, sizeof(u32));
> > break;
> > case 'l':
> > p = special_hex_number(p, output + sizeof(output) - 2, le32_to_cpu(orig_le), sizeof(u32));
> > break;
> > case 'b':
> > p = special_hex_number(p, output + sizeof(output) - 2, be32_to_cpu(orig_be), sizeof(u32));
> > break;
> > }
> >
> > *p++ = ')';
> > *p = '\0';
> >
> > return string(buf, end, output, spec);
> > }
> >
> > Andy, could you verify this?
>
> Looking at the header files, it looks like doing cpu_to_le32 on that variable
> and doing le32_to_cpu will actually reverse the order twice, on big endian
> systems, thus technically all way would not swap the order at all.
>
> I'm not really sure how to manage the sparse warnings here.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists