linux-kernel - Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

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: <926da814-b21d-d01d-d0bb-6304bf545218@rasmusvillemoes.dk>
Date:   Thu, 7 Mar 2019 21:28:45 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Kees Cook <keescook@...omium.org>,
        Leon Romanovsky <leonro@...lanox.com>
Cc:     Bart Van Assche <bvanassche@....org>,
        Jason Gunthorpe <jgg@...lanox.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler
 warning when building with W=1

On 07/03/2019 18.12, Kees Cook wrote:
> On Thu, Mar 7, 2019 at 9:02 AM Leon Romanovsky <leonro@...lanox.com> wrote:
>>
>> On Thu, Mar 07, 2019 at 08:52:51AM -0800, Kees Cook wrote:
>>> On Thu, Mar 7, 2019 at 7:40 AM Leon Romanovsky <leonro@...lanox.com> wrote:
>>>>
>>>> On Thu, Mar 07, 2019 at 06:53:54AM -0800, Bart Van Assche wrote:
>>>>> On 3/6/19 11:24 PM, Leon Romanovsky wrote:
>>>>>> My simple patch passes too :).
>>>>>
>>>>> Can you repost your patch?
>>>>
>>>> https://patchwork.kernel.org/patch/10841079/
>>>>
>>>> As Rasmus wrote, the thing is to avoid a < 0 check. In my patch,
>>>> I converted a <= 0 to !(a > 0 || a == 0) expression.
>>>
>>> I'd be happy either way. Is there a larger benefit to having a safe
>>> "is_non_negative()" helper, or should we go with the minimal change to
>>> the shl macro?
>>
>> I personally prefer simplest possible solution.

So, I played around with a few variants on godbolt.org, and it seems
that gcc is smart enough to combine (a > 0 || a == 0) into (a >= 0) - in
all the cases I tried Leon's patch resulted in the exact same generated
code as the current version. Conversely, and rather surprising to me,
Bart's patch seemed to cause worse code generation. So now I've changed
my mind and also support Leon's version - however, I would _strongly_
prefer if it introduced

#define is_non_negative(a) (a > 0 || a == 0)
#define is_negative(a) (!(is_non_negative(a))

with appropriate comments and used that. check_shl_overflow is hard
enough to read already.

Rasmus