From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935207AbeEISHX (ORCPT ); Wed, 9 May 2018 14:07:23 -0400 Received: from mail-ua0-f196.google.com ([209.85.217.196]:46832 "EHLO mail-ua0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933490AbeEISHV (ORCPT ); Wed, 9 May 2018 14:07:21 -0400 X-Google-Smtp-Source: AB8JxZr/ETvR4p6/j48cf5nWHcZRjO6QNn5OYq48MMDK2DP4vlC/j7jqIej/Lx2s8k2W8rIbsMFJGqNID+nmhamiy8Q= MIME-Version: 1.0 In-Reply-To: References: <20180509004229.36341-1-keescook@chromium.org> <20180509004229.36341-5-keescook@chromium.org> <20180509113446.GA18549@bombadil.infradead.org> From: Kees Cook Date: Wed, 9 May 2018 11:07:20 -0700 X-Google-Sender-Auth: j5OPLfJOXdsPPg5OS_BYXIn08-g Message-ID: Subject: Re: [PATCH 04/13] mm: Use array_size() helpers for kmalloc() To: Rasmus Villemoes Cc: Matthew Wilcox , Matthew Wilcox , LKML , Linux-MM , Kernel Hardening Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 9, 2018 at 11:00 AM, Rasmus Villemoes 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