lkml.org 
[lkml]   [2018]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [GIT PULL] overflow updates (part 2) for v4.18-rc1
On Tue, Jun 12, 2018 at 6:44 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jun 12, 2018 at 4:36 PM Kees Cook <keescook@chromium.org> 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

\
 
 \ /
  Last update: 2018-06-13 21:08    [W:0.028 / U:3.132 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site