linux-kernel - Re: [PATCH 04/13] mm: Use array

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: <CAGXu5j+ds=4tKM=GwzgfpA0YheUSX30pTbQqXg0gKUniOeDzww@mail.gmail.com>
Date:   Wed, 9 May 2018 11:07:20 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Matthew Wilcox <mawilcox@...rosoft.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Kernel Hardening <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH 04/13] mm: Use array_size() helpers for kmalloc()

On Wed, May 9, 2018 at 11:00 AM, Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
> On 2018-05-09 13:34, Matthew Wilcox wrote:
>> On Tue, May 08, 2018 at 05:42:20PM -0700, Kees Cook wrote:
>>> @@ -499,6 +500,8 @@ static __always_inline void *kmalloc_large(size_t size, gfp_t flags)
>>>   */
>>>  static __always_inline void *kmalloc(size_t size, gfp_t flags)
>>>  {
>>> +    if (size == SIZE_MAX)
>>> +            return NULL;
>>>      if (__builtin_constant_p(size)) {
>>>              if (size > KMALLOC_MAX_CACHE_SIZE)
>>>                      return kmalloc_large(size, flags);
>>
>> I don't like the add-checking-to-every-call-site part of this patch.
>> Fine, the compiler will optimise it away if it can calculate it at compile
>> time, but there are a lot of situations where it can't.  You aren't
>> adding any safety by doing this; trying to allocate SIZE_MAX bytes is
>> guaranteed to fail, and it doesn't need to fail quickly.
>
> Yeah, agree that we don't want to add a size check to all callers,
> including those where the size doesn't even come from one of the new
> *_size helpers; that just adds bloat. It's true that the overflow case
> does not have to fail quickly, but I was worried that the saturating
> helpers would end up making gcc emit a cmov instruction, thus stalling
> the regular path. But it seems that it actually ends up doing a forward
> jump, sets %rdi to SIZE_MAX, then jumps back to the call of __kmalloc,
> so it should be ok.

Okay, consensus is to remove new SIZE_MAX checks, then?

> With __builtin_constant_p(size) && size == SIZE_MAX, gcc could be smart
> enough to elide those two instructions and have the jo go directly to
> the caller's error handling, but at least gcc 5.4 doesn't seem to be
> that smart. So let's just omit that part for now.
>
> But in case of the kmalloc_array functions, with a direct call of
> __builtin_mul_overflow(), gcc does combine the "return NULL" with the
> callers error handling, thus avoiding the six byte "%rdi = -1; jmp
> back;" thunk. That, along with the churn factor, might be an argument
> for leaving the current callers of *_array alone. But if we are going to
> keep those longer-term, we might as well convert kmalloc(a, b) into
> kmalloc_array(a, b) instead of kmalloc(array_size(a, b)). In any case, I
> do see the usefulness of the struct_size helper, and agree that we
> definitely should not introduce a new *_struct variant that needs to be
> implemented in all families.

I'd like to drop *calloc() and *_array() to simplify APIs (and improve
developer sanity). Are you suggesting we should not use the overflow
helpers in kmalloc_array(), instead leaving the existing open-coded
overflow check?

-Kees

-- 
Kees Cook
Pixel Security