linux-kernel - RE: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove the restore_c_regs_and

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: <ebc17ddaae904cae96f2da3054a239e8@ustx2ex-dag1mb5.msg.corp.akamai.com>
Date:   Thu, 7 Sep 2017 20:41:38 +0000
From:   "Lubashev, Igor" <ilubashe@...mai.com>
To:     "Pai, Vishwanath" <vpai@...mai.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
CC:     Ingo Molnar <mingo@...nel.org>, "Hunt, Joshua" <johunt@...mai.com>,
        "Pablo Neira Ayuso" <pablo@...filter.org>,
        Borislav Petkov <bp@...en8.de>,
        "Andy Lutomirski" <luto@...nel.org>,
        the arch/x86 maintainers <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Brian Gerst <brgerst@...il.com>,
        Andrew Cooper <andrew.cooper3@...rix.com>,
        Juergen Gross <jgross@...e.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "David S. Miller" <davem@...emloft.net>,
        Arnd Bergmann <arnd@...db.de>
Subject: RE: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove
 the restore_c_regs_and_iret label)

Since user is u64, it is best to have a predictable return value for all possible values of user.  So maybe:

static u64 user2rate_bytes(u64 user)
{
        u64 r;

        r = user ? U32_MAX / (u32) min(user, U32_MAX) : U32_MAX;
        r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT;
        return r;
}


-----Original Message-----
From: Vishwanath Pai [mailto:vpai@...mai.com] 
Sent: Thursday, September 07, 2017 4:17 PM
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Ingo Molnar <mingo@...nel.org>; Lubashev, Igor <ilubashe@...mai.com>; Hunt, Joshua <johunt@...mai.com>; Pablo Neira Ayuso <pablo@...filter.org>; Borislav Petkov <bp@...en8.de>; Andy Lutomirski <luto@...nel.org>; the arch/x86 maintainers <x86@...nel.org>; Linux Kernel Mailing List <linux-kernel@...r.kernel.org>; Brian Gerst <brgerst@...il.com>; Andrew Cooper <andrew.cooper3@...rix.com>; Juergen Gross <jgross@...e.com>; Boris Ostrovsky <boris.ostrovsky@...cle.com>; Kees Cook <keescook@...omium.org>; Andrew Morton <akpm@...ux-foundation.org>; David S. Miller <davem@...emloft.net>; Arnd Bergmann <arnd@...db.de>
Subject: Re: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove the restore_c_regs_and_iret label)

On 09/07/2017 02:43 PM, Linus Torvalds wrote:
> Note: that patch has *exactly* the issue I was talking about above.
> 
> Doing that
> 
>     if (user > 0xFFFFFFFFULL)
>         return 0;
> 
> is different from the old code, which used to result in a zero in the 
> divide, and then
> 
>     r = (r - 1) << 4;
> 
> would cause it to return a large value.
> 
> So the patch in question doesn't just fix the build error, it 
> completely changes the semantics of the function too.
> 
> I *think* the new behavior is likely what you want, but these kinds of 
> things should be _described_.
> 
> Also, even with the patch, we have garbage:
> 
>     0xFFFFFFFFULL / (u32)user
> 
> why is that sub-expression pointlessly doing a 64-bit divide with a 
> 32-bit number? The compiler is hopefully smart enough to point things 
> out, but that "ULL" really is _wrong_ there, and could cause a stupid 
> compiler to still do a 64-bit divide (although hopefully the simpler 
> version that is 64/32).
> 
> So please clarify both the correct behavior _and_ the actual typing of 
> the divide, ok?
> 
>                  Linus

The value of 'user' is sent from userspace, which is the return value of this function:

static uint64_t bytes_to_cost(uint32_t bytes) {
        uint32_t r = bytes >> XT_HASHLIMIT_BYTE_SHIFT;
        return UINT32_MAX / (r+1);
}

What user2rate_bytes() is trying to do is the opposite of above. The size of 'user' is 64bit for a different reason altogether, but in this case it is guaranteed to be always < U32_MAX. And hence using 64bit divide is completely pointless (which I now realize).

Writing U32INT_MAX as 0xFFFFFFFFULL was a mistake on my part. I could have avoided all of this by using built-in constants instead of trying to define them myself. I will rewrite the function as below and send out another patch:

static u64 user2rate_bytes(u64 user)
{
        u64 r;

        r = user ? U32_MAX / (u32) user : U32_MAX;
        r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT;
        return r;
}

-Vishwanath