linux-kernel - Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs

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: <20200403095441.GL1922688@smile.fi.intel.com>
Date:   Fri, 3 Apr 2020 12:54:41 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Petr Mladek <pmladek@...e.com>, linux-media@...r.kernel.org,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        hverkuil@...all.nl, laurent.pinchart@...asonboard.com,
        mchehab@...nel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Joe Perches <joe@...ches.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>
Subject: Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and
 DRM fourccs

On Fri, Apr 03, 2020 at 12:39:16PM +0300, Sakari Ailus wrote:
> On Fri, Apr 03, 2020 at 12:31:03PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 03, 2020 at 12:11:56PM +0300, Sakari Ailus wrote:
> > > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > > the same implementation can be used.
> > 
> > ...
> > 
> > > +static noinline_for_stack
> > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > +		    struct printf_spec spec, const char *fmt)
> > > +{
> > 
> > > +#define FOURCC_STRING_BE	"-BE"
> > > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > 
> > I guess it makes it too complicated.
> 
> The above also clearly binds the size to the data that is expected to
> contain there. I'd prefer keeping it as-is. And yes, 8 would be correct,
> too.

OK.

> > 	char s[8];
> > 
> > > +	if (check_pointer(&buf, end, fourcc, spec))
> > > +		return buf;
> > > +
> > > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > +		return error_string(buf, end, "(%p4?)", spec);
> > > +
> > 
> > > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > 
> > Can you elaborate what the difference in output with this bit set over cleared?
> > I.o.w. why don't we need to put it as BE and for LE case addd "-LE"?
> 
> The established practice is that big endian formats have "-BE" suffix
> whereas the little endian ones have nothing. (At least when it comes to
> V4L2.)

What I meant by the first part of the question is ordering of the characters.
That ordering of characters is not related to that flag, correct? So, bit
actually defines the endianess of the data in the certain fourcc.

Probably you need to put a comment to explain this.

> > > +	if (*fourcc & BIT(31))
> > > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > +			sizeof(FOURCC_STRING_BE));
> > 
> > We know the size, and we may put '\0' as well
> > 	if (*fourcc & BIT(31))
> > 		strscpy(&s[4], "-BE", sizeof("-BE"));
> > 	else
> > 		strscpy(&s[4], "", sizeof(""));
> 
> The rest of the struct memory has already been set to zero in variable
> declaration.

Which is bogus in my opinion. strscpy() or direct '\0' termination will put it
more explicit.

-- 
With Best Regards,
Andy Shevchenko