From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Kees Cook <keescook@chromium.org>
Cc: Matthew Wilcox <willy@infradead.org>,
Matthew Wilcox <mawilcox@microsoft.com>,
LKML <linux-kernel@vger.kernel.org>,
Linux-MM <linux-mm@kvack.org>,
Kernel Hardening <kernel-hardening@lists.openwall.com>
Subject: Re: [PATCH 04/13] mm: Use array_size() helpers for kmalloc()
Date: Wed, 9 May 2018 20:39:35 +0200 [thread overview]
Message-ID: <f761626f-03e5-ef91-1f73-ae1cf2c9714f@rasmusvillemoes.dk> (raw)
In-Reply-To: <CAGXu5j+ds=4tKM=GwzgfpA0YheUSX30pTbQqXg0gKUniOeDzww@mail.gmail.com>
On 2018-05-09 20:07, Kees Cook wrote:
> On Wed, May 9, 2018 at 11:00 AM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> Okay, consensus is to remove new SIZE_MAX checks, then?
Yes, don't add such to static inlines. But the out-of-line
implementations do need an audit (as you've observed) for unsafe
arithmetic on the passed-in size.
>> 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?
No, quite the contrary. I suggest using check_mul_overflow() directly in
kmalloc_array (and by implication, kcalloc), and also all other *_array
or *_calloc that are static inlines. That's separate from converting
kmalloc(a*b) to use some safer variant, and should not be controversial
(and can generate better code for all the existing callers).
Now, what kmalloc(a*b) should be converted to is a question of the
long-term plans for *_array. If you want to remove it completely,
eventually, it doesn't make sense to coccinel (yeah, that's a verb) in
new users.
And a third question is whether and when to mechanically change all
(pre-)existing kmalloc_array() into kmalloc(array_size()). I don't have
an opinion on the latter two.
Rasmus
next prev parent reply other threads:[~2018-05-09 18:39 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-09 0:42 [RFC][PATCH 00/13] Provide saturating helpers for allocation Kees Cook
2018-05-09 0:42 ` [PATCH 01/13] compiler.h: enable builtin overflow checkers and add fallback code Kees Cook
2018-05-09 0:42 ` [PATCH 02/13] lib: add runtime test of check_*_overflow functions Kees Cook
2018-05-09 0:42 ` [PATCH 03/13] overflow.h: Add allocation size calculation helpers Kees Cook
2018-05-09 18:27 ` Rasmus Villemoes
2018-05-09 18:49 ` Kees Cook
2018-05-09 0:42 ` [PATCH 04/13] mm: Use array_size() helpers for kmalloc() Kees Cook
2018-05-09 11:34 ` Matthew Wilcox
2018-05-09 17:58 ` Kees Cook
2018-05-09 18:00 ` Rasmus Villemoes
2018-05-09 18:07 ` Kees Cook
2018-05-09 18:39 ` Rasmus Villemoes [this message]
2018-05-09 0:42 ` [PATCH 05/13] mm: Use array_size() helpers for kvmalloc() Kees Cook
2018-05-09 0:42 ` [PATCH 06/13] treewide: Use struct_size() for kmalloc()-family Kees Cook
2018-05-09 0:42 ` [PATCH 07/13] treewide: Use struct_size() for vmalloc()-family Kees Cook
2018-05-09 0:42 ` [PATCH 08/13] treewide: Use struct_size() for devm_kmalloc() and friends Kees Cook
2018-05-09 0:42 ` [PATCH 09/13] treewide: Use array_size() for kmalloc()-family Kees Cook
2018-05-09 0:42 ` [PATCH 10/13] treewide: Use array_size() for kmalloc()-family, leftovers Kees Cook
2018-05-09 0:42 ` [PATCH 11/13] treewide: Use array_size() for vmalloc() Kees Cook
2018-05-09 0:42 ` [PATCH 12/13] treewide: Use array_size() for devm_*alloc()-like Kees Cook
2018-05-09 0:42 ` [PATCH 13/13] treewide: Use array_size() for devm_*alloc()-like, leftovers Kees Cook
2018-05-09 16:08 ` [RFC][PATCH 00/13] Provide saturating helpers for allocation Laura Abbott
2018-05-09 17:01 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f761626f-03e5-ef91-1f73-ae1cf2c9714f@rasmusvillemoes.dk \
--to=linux@rasmusvillemoes.dk \
--cc=keescook@chromium.org \
--cc=kernel-hardening@lists.openwall.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mawilcox@microsoft.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).