linux-kernel - Re: [PATCH v3] media: si2168: Refactor command setup code

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 13 Jul 2019 00:11:12 +0200
From:   Marc Gonzalez <marc.w.gonzalez@...e.fr>
To:     Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Brad Love <brad@...tdimension.cc>
Cc:     Antti Palosaari <crope@....fi>,
        Jonathan Neuschäfer <j.neuschaefer@....net>,
        linux-media <linux-media@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        MSM <linux-arm-msm@...r.kernel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
Subject: Re: [PATCH v3] media: si2168: Refactor command setup code

On 12/07/2019 19:45, Mauro Carvalho Chehab wrote:

> Brad Love <brad@...tdimension.cc> escreveu:
> 
>> On 04/07/2019 05.33, Marc Gonzalez wrote:
>>
>>> +#define CMD_SETUP(cmd, args, rlen) \
>>> +	cmd_setup(cmd, args, sizeof(args) - 1, rlen)
>>> +  
>>
>> This is only a valid helper if args is a null terminated string. It just
>> so happens that every instance in this driver is, but that could be a
>> silent pitfall if someone used a u8 array with this macro.
> 
> Actually, it is uglier than that. If one writes something like:
> 
> 	char buf[20];
> 
> 	buf[0] = 0x20;
> 	buf[1] = 0x03;
> 
> 	CMD_SETUP(cmd, buf, 0);
> 
> 	// some other init, up to 5 values, then another CMD_SETUP()

I'm not sure what you mean in the // comment.
What kind of init? Why up to 5 values? Why another CMD_SETUP?

> sizeof() will evaluate to 20, and not to 2, with would be the
> expected buffer size, and it will pass 18 random values.
> 
> IMHO, using sizeof() here is a very bad idea.

You may have a point...
(Though I'm not proposing a kernel API function, merely code
refactoring for a single file that's unlikely to change going
forward.)

It's also bad form to repeat the cmd size (twice) when the compiler
can figure it out automatically for string literals (which is 95%
of the use-cases).

I can drop the macro, and just use the helper...

Or maybe there's a GCC extension to test that an argument is a
string literal...

Regards.