Re: [GIT PULL] overflow updates (part 2) for v4.18-rc1
From: Kees Cook
Date: Wed Jun 13 2018 - 15:07:36 EST
On Tue, Jun 12, 2018 at 6:44 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Jun 12, 2018 at 4:36 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>>
>> - Treewide conversions of allocators to use either 2-factor argument
>> variant when available, or array_size() and array3_size() as needed (Kees)
>
> Ok, some of this smells just a tad too much of automation, but I've
> done the pull and it's going through my build tests.
Thanks! Yeah, I tried to beat sense into it to avoid REALLY dumb
things that just looked terrible.
> In some of the cases it turns a compile-time constant into a function
> call, ie this just makes for bigger and slower code for no reason
> what-so-ever.
>
> - ch->tx_array = vzalloc(MIC_DMA_DESC_RX_SIZE * sizeof(*ch->tx_array));
> + ch->tx_array = vzalloc(array_size(MIC_DMA_DESC_RX_SIZE,
> + sizeof(*ch->tx_array)));
>
> At least in the kzalloc/kcalloc conversion it results in more legible code.
I did try to convince my scripting to avoid the less sane conversions,
but as you saw there were still some that fell through. In the end, I
decided to let it stand since because Rasmus's code is so careful, if
array_size() processes constant expressions, it'll produce a constant
expression, so the machine code result actually isn't worse in these
cases. Using the above example, it's the same as without array_size():
struct dma_async_tx_descriptor is 72 bytes
/* size: 72, cachelines: 2, members: 10 */
MIC_DMA_DESC_RX_SIZE is 131068
#define MIC_DMA_DESC_RX_SIZE (128 * 1024 - 4)
131068 * 72 = 0x8ffee0
vzalloc(array_size(MIC_DMA_DESC_RX_SIZE, sizeof(*ch->tx_array)))
ffffffff815e87d0: bf e0 fe 8f 00 mov $0x8ffee0,%edi
ffffffff815e87d5: e8 c6 4b be ff callq <vzalloc>
The same is true for each of these all-constants forms, with each
resolving to the same machine code in my tests:
kmalloc(16 * PAGE_SIZE, GFP_KERNEL)
kmalloc_array(16, PAGE_SIZE, GFP_KERNEL)
kmalloc(array_size(16, PAGE_SIZE), GFP_KERNEL)
16 * 4096 = 0x10000
ffffffff8179810e: ba 04 00 00 00 mov $0x4,%edx
ffffffff81798113: be c0 00 60 00 mov $0x6000c0,%esi
ffffffff81798118: bf 00 00 01 00 mov $0x10000,%edi
ffffffff8179811d: e8 6e b0 a1 ff callq <kmalloc_order_trace>
Obviously, it gets more interesting once there is an actual variable in play:
kmalloc(var * PAGE_SIZE, GFP_KERNEL) does no overflow checking, as
expected, and is what I wanted to eliminate from the kernel:
ffffffff8179810e: 48 63 3d 93 f4 14 01 movslq 0x114f493(%rip),%rdi
ffffffff81798115: be c0 00 60 00 mov $0x6000c0,%esi
ffffffff8179811a: 48 c1 e7 0c shl $0xc,%rdi
ffffffff8179811e: e8 1d af a4 ff callq <__kmalloc>
kmalloc_array(var, PAGE_SIZE, GFP_KERNEL) has its builtin overflow
checking and returns NULL:
ffffffff8179810e: 48 63 05 93 f4 14 01 movslq 0x114f493(%rip),%rax
ffffffff81798115: bf 00 10 00 00 mov $0x1000,%edi
ffffffff8179811a: 48 f7 e7 mul %rdi
ffffffff8179811d: 48 89 c7 mov %rax,%rdi
ffffffff81798120: 70 18 jo ffffffff8179813a
ffffffff81798122: be c0 00 60 00 mov $0x6000c0,%esi
ffffffff81798127: e8 14 af a4 ff callq <__kmalloc>
ffffffff8179812c: 48 89 05 ad ea fd 01 mov %rax,0x1fdeaad(%rip)
ffffffff81798133: 48 83 c4 08 add $0x8,%rsp
ffffffff81798137: 5b pop %rbx
ffffffff81798138: 5d pop %rbp
ffffffff81798139: c3 retq
ffffffff8179813a: 31 c0 xor %eax,%eax
ffffffff8179813c: eb ee jmp ffffffff8179812c
kmalloc(array_size(var, PAGE_SIZE), GFP_KERNEL), (not that this form
should be used, but just to illustrate) performs the saturation
instead of the NULL return:
ffffffff8179810e: 48 63 05 93 f4 14 01 movslq 0x114f493(%rip),%rax
ffffffff81798115: bf 00 10 00 00 mov $0x1000,%edi
ffffffff8179811a: 48 f7 e7 mul %rdi
ffffffff8179811d: 48 89 c7 mov %rax,%rdi
ffffffff81798120: 70 18 jo ffffffff8179813a
ffffffff81798122: be c0 00 60 00 mov $0x6000c0,%esi
ffffffff81798127: e8 14 af a4 ff callq <__kmalloc>
ffffffff8179812c: 48 89 05 ad ea fd 01 mov %rax,0x1fdeaad(%rip)
ffffffff81798133: 48 83 c4 08 add $0x8,%rsp
ffffffff81798137: 5b pop %rbx
ffffffff81798138: 5d pop %rbp
ffffffff81798139: c3 retq
ffffffff8179813a: ba 34 00 00 00 mov $0x34,%edx
ffffffff8179813f: be c0 00 60 00 mov $0x6000c0,%esi
ffffffff81798144: 48 83 cf ff or $0xffffffffffffffff,%rdi
ffffffff81798148: e8 43 b0 a1 ff callq <kmalloc_order_trace>
ffffffff8179814d: eb dd jmp ffffffff8179812c
> To make up for it, there's some conversions *away* from nonsensical expressions:
>
> - hc_name = kzalloc(sizeof(char) * (HSMMC_NAME_LEN + 1), GFP_KERNEL);
> + hc_name = kzalloc(HSMMC_NAME_LEN + 1, GFP_KERNEL);
Yeah, I tried to catch these and some other masked cases. Coccinelle
didn't seem to have a consistent isomorphism for (sizeof(thing)) vs
sizeof(thing), so I also tried to drop redundant parens.
> but I _really_ think you were way too eager to move to array_size()
> even when it made no sense what-so-ever.
>
> But at least with the kcalloc()/kmalloc_array() conversions now
> preferred, those crazy cases are now a minority rather than "most of
> the patch makes code worse".
Net improvement was my goal, yes! :)
> I am *not* looking forward to the conflicts this causes, but maybe it
> won't be too bad. Fingers crossed.
Hopefully it shouldn't be too bad, but that's why I tried to perform
the conversion as late in -rc1 as possible, etc.
Thanks again for the pull! I'll keep my eyes out for new cases that
need conversion. Hopefully we can enhance checkpatch to yell more
loudly too. :)
-Kees
--
Kees Cook
Pixel Security