Re: [GIT PULL] overflow updates (part 2) for v4.18-rc1

[Kees Cook <keescook@chromium.org>]

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