Messages in this thread | | | From | Kees Cook <> | Date | Wed, 13 Jun 2018 12:07:29 -0700 | Subject | Re: [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
| |