netdev - Re: [PATCH iproute2 v2 1/2] bpf: Fix segfault when custom pinning is used

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: <b1b03d43-5f89-be71-4d74-b18a1d7d69d4@mojatatu.com>
Date:   Thu, 23 Apr 2020 13:46:44 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Dominique Martinet <asmadeus@...ewreck.org>
Cc:     Stephen Hemminger <stephen@...workplumber.org>,
        netdev@...r.kernel.org, dsahern@...il.com, aclaudi@...hat.com,
        daniel@...earbox.net, Jamal Hadi Salim <hadi@...atatu.com>
Subject: Re: [PATCH iproute2 v2 1/2] bpf: Fix segfault when custom pinning is
 used

On 2020-04-23 2:30 a.m., Dominique Martinet wrote:
> Jamal Hadi Salim wrote on Wed, Apr 22, 2020:

[..]
>>> Initializing the whole string to 0 is over kill here.
>>
>> Why is it overkill? ;->
>> Note: I just replicated other parts of the same file which
>> initialize similar array to 0.
> 
> FWIW I kind of agree this is overkill, there's only one other occurence
> of a char * being explicitely zeroed, the rest isn't strings so probably
> have better reasons to.

But "overkill" is such a strong word;-> Is it affecting performance,
readability, or is the point that it was over-engineering exercise?
Do note: there's other code that does the same thing (even in
the same file!). And all i did was, in TheLinuxWay(tm),
just cutnpasted.
Same thing with checking for return code of snprintf.

Consistency is the problem - because there are different styles
in the same tree. Possibly had i sent the code using the suggested
approach someone would have probably pointed out i shouldve used
the approach i did;->
In any case, i think we are now heading in the direction
of the bike shed ;->

I will fix this and snprintf in the next update.

> 
> snprintf will safely zero-terminate it and nothing should ever access
> past the nul byte so it shouldn't be necessary.
> 

It would also depend on the knowledge of the coder on what could
go wrong.
You may still want to know what you think you wrote in there was
picked up (so checking max size)..
I dont know why the manpage says you'll get a negative return
but the safest thing against Murphy should be to check for all
possible boundary conditions.

Note: The original fix from Andrea was for a compiler warning on
snprintf.

I will send the next update..

cheers,
jamal