linux-kernel - Re: [PATCH] EDAC: remove unnecessary static in edac_fake_inject

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: <6867696d-edd1-3d21-eb7e-af68e087750a@embeddedor.com>
Date:   Sun, 23 Jul 2017 00:42:38 -0500
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Julia Lawall <julia.lawall@...6.fr>,
        "Gustavo A. R. Silva" <garsilva@...eddedor.com>
Cc:     Borislav Petkov <bp@...en8.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org,
        cocci@...teme.lip6.fr
Subject: Re: [PATCH] EDAC: remove unnecessary static in
 edac_fake_inject_write()

Hi Julia,

On 07/23/2017 12:07 AM, Julia Lawall wrote:
>
>
> On Sat, 22 Jul 2017, Gustavo A. R. Silva wrote:
>
>> Hi Julia, Borislav,
>>
>> On 07/22/2017 11:22 AM, Gustavo A. R. Silva wrote:
>>> Hi all,
>>>
>>> On 07/22/2017 01:36 AM, Borislav Petkov wrote:
>>>> On Fri, Jul 21, 2017 at 10:08:12PM +0200, Julia Lawall wrote:
>>>>> Someone pointed out that the rule is probably not OK when the address of
>>>>> the static variable is taken, because then it is likely being used as
>>>>> permanent storage.
>>>>
>>>> Makes sense to me.
>>>>
>>>>> An improved rule is:
>>>>
>>>> Do you think it is worth having it in scripts/coccinelle/ ?
>>>>
>>>> I don't think Gustavo would mind putting it there :)
>>>>
>>>
>>> Absolutely, I'd be glad to help out. :)
>>>
>>
>> I've been working on this issue today and, in my opinion, this script is even
>> better:
>>
>> @bad exists@
>> position p;
>> identifier x;
>> expression e;
>> type T;
>> @@
>>
>> static T x@p;
>> ... when != x = e
>> x = <+...x...+>
>>
>> @worse1 exists@
>> position p;
>> identifier x;
>> type T;
>> @@
>>
>> static T x@p;
>> ...
>> return &x;
>>
>> @worse2 exists@
>> position p;
>> identifier x;
>> type T;
>> @@
>>
>> static T *x@p;
>> ...
>> return x;
>>
>> @@
>> identifier x;
>> expression e;
>> type T;
>> position p != {bad.p,worse1.p,worse2.p};
>> @@
>>
>> -static
>>   T x@p;
>>   ... when != x
>>       when strict
>> ?x = e;
>>
>> It ignores all the cases in which the address of the static variable is
>> returned to the caller function.
>
> I don't understand why you want to restrict the address of a variable case
> to returns.  Storing the address in a field of a structure that has a
> lifetime beyond the function body is a problem as well.
>

Yeah, I totally agree and, personally I consider that a bad coding 
practice. But I think those kinds of issues should be addressed in a 
different script.

> On the other hand returning the value stored in a static variable is not a
> problem.  That value exists independently of the variable that contains
> it.  The variable that conains it doesn't need to live on in any way.
>

Yeah, I agree, but I don't see exactly where this argument is coming from ?

Notice that for both worse1 and worse2, what is returned is the address, 
not the value of the static variable. At least that was my intention, 
unless I maybe missing something ?

>>
>> Also, there are some cases in which the maintainer can argue something like
>> the following:
>>
>> https://lkml.org/lkml/2017/7/19/1381
>>
>> but that depends on the particular conditions in which the code is intended to
>> be executed.
>>
>> What do you think?
>
> The preserving values argument is not relevant.  The rule checks that the
> value is never used.  DMA accesses should involve taking an address, which
> we now disallow.  It seems likely that anything large would have its
> address taken too, but one could check manually for that.  spgen provides
> a section where you can describe such issues.
>

Yeah, those cases should be analyzed manually and in case of doubt check 
with the maintainers.

Thanks
-- 
Gustavo A. R. Silva