Messages in this thread | | | From | Kees Cook <> | Date | Mon, 30 Apr 2018 15:29:39 -0700 | Subject | Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct |
| |
On Mon, Apr 30, 2018 at 1:16 PM, Matthew Wilcox <willy@infradead.org> wrote: > On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote: >> For any longer multiplications, I've only found[1]: >> >> drivers/staging/rtl8188eu/os_dep/osdep_service.c: void **a = >> kzalloc(h * sizeof(void *) + h * w * size, GFP_KERNEL); > > That's pretty good, although it's just an atrocious vendor driver and > it turns out all of those things are constants, and it'd be far better > off with just declaring an array. I bet they used to declare one on > the stack ...
Yeah, it was just a quick hack to look for stuff.
> >> At the end of the day, though, I don't really like having all these >> different names... >> >> kmalloc(), kmalloc_array(), kmalloc_ab_c(), kmalloc_array_3d() >> >> with their "matching" zeroing function: >> >> kzalloc(), kcalloc(), kzalloc_ab_c(), kmalloc_array_3d(..., gfp | __GFP_ZERO) > > Yes, it's not very regular. > >> For the multiplication cases, I wonder if we could just have: >> >> kmalloc_multN(gfp, a, b, c, ...) >> kzalloc_multN(gfp, a, b, c, ...) >> >> and we can replace all kcalloc() users with kzalloc_mult2(), all >> kmalloc_array() users with kmalloc_mult2(), the abc uses with >> kmalloc_mult3(). > > I'm reluctant to do away with kcalloc() as it has the obvious heritage > from user-space calloc() with the addition of GFP flags.
But it encourages misuse with calloc(N * M, gfp) ... if we removed calloc and kept k[mz]alloc_something(gfp, a, b, c...) I think we'd have better adoption.
>> That said, I *do* like kmalloc_struct() as it's a very common pattern... > > Thanks! And way harder to misuse than kmalloc_ab_c().
Yes, quite so. It's really why I went with kmalloc_array_3d(), but now I'm thinking better of it...
>> Or maybe, just leave the pattern in the name? kmalloc_ab(), >> kmalloc_abc(), kmalloc_ab_c(), kmalloc_ab_cd() ? >> >> Getting the constant ordering right could be part of the macro >> definition, maybe? i.e.: >> >> static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags) >> { >> if (__builtin_constant_p(a) && a != 0 && \ >> b > SIZE_MAX / a) >> return NULL; >> else if (__builtin_constant_p(b) && b != 0 && \ >> a > SIZE_MAX / b) >> return NULL; >> >> return kmalloc(a * b, flags); >> } > > Ooh, if neither a nor b is constant, it just didn't do a check ;-( This > stuff is hard.
Yup, quite true. Obviously not the final form. ;) I meant to illustrate that we could do compile-time tricks to reorder the division in an efficient manner.
>> (I just wish C had a sensible way to catch overflow...) > > Every CPU I ever worked with had an "overflow" bit ... do we have a > friend on the C standards ctte who might figure out a way to let us > write code that checks it?
On the CPU it's not retained across multiple calculations. And the type matters too. This came up recently in a separate thread too: http://openwall.com/lists/kernel-hardening/2018/03/26/4
>> [1] git grep -E 'alloc\([^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+,' > > I'm impressed, but it's not going to catch > > veryLongPointerNameThatsMeaningfulToMe = kmalloc(initialSize + > numberOfEntries * entrySize + someOtherThing * yourMum, > GFP_KERNEL);
Right, it wasn't meant to be exhaustive. I just included it in case anyone wanted to go grepping around for themselves.
-Kees
-- Kees Cook Pixel Security
| |