Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751415AbdGNGN4 (ORCPT ); Fri, 14 Jul 2017 02:13:56 -0400 Received: from mail-oi0-f45.google.com ([209.85.218.45]:35141 "EHLO mail-oi0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750819AbdGNGNz (ORCPT ); Fri, 14 Jul 2017 02:13:55 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170706220114.142438-1-ghackmann@google.com> <20170706220114.142438-2-ghackmann@google.com> From: Dmitry Vyukov Date: Fri, 14 Jul 2017 08:13:33 +0200 Message-ID: Subject: Re: [PATCH 1/4] kasan: support alloca() poisoning To: Greg Hackmann Cc: Andrey Ryabinin , Alexander Potapenko , Masahiro Yamada , Michal Marek , LKML , kasan-dev , "linux-mm@kvack.org" , "open list:KERNEL BUILD + fi..." , Matthias Kaehlcke , Michael Davidson Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2538 Lines: 63 On Fri, Jul 14, 2017 at 12:40 AM, Greg Hackmann wrote: > Hi, > > Thanks for taking a look at this patchstack. I apologize for the delay in > responding. > > On 07/10/2017 01:44 AM, Dmitry Vyukov wrote: >>> >>> + >>> + const void *left_redzone = (const void *)(addr - >>> + KASAN_ALLOCA_REDZONE_SIZE); >>> + const void *right_redzone = (const void *)(addr + >>> rounded_up_size); >> >> >> Please check that size is rounded to KASAN_ALLOCA_REDZONE_SIZE. That's >> the expectation, right? That can change is clang silently. >> >>> + kasan_poison_shadow(left_redzone, KASAN_ALLOCA_REDZONE_SIZE, >>> + KASAN_ALLOCA_LEFT); >>> + kasan_poison_shadow(right_redzone, >>> + padding_size + KASAN_ALLOCA_REDZONE_SIZE, >>> + KASAN_ALLOCA_RIGHT); >> >> >> We also need to poison the unaligned part at the end of the object >> from size to rounded_up_size. You can see how we do it for heap >> objects. > > > The expectation is that `size' is the exact size of the alloca()ed object. > `rounded_up_size' then adds the 0-7 bytes needed to adjust the size to the > ASAN shadow scale. So `addr + rounded_up_size' should be the correct place > to start poisoning. We need to start poisoning at addr+size exactly. Asan shadow scheme supports this. It's not possible to poison beginning of an aligned 8-byte block, but leave tail unpoisoned. But it is possible to poison tail of an aligned 8-byte block and leave beginning unpoisoned. Look at what we do for kmalloc. > In retrospect this part of the code was pretty confusing. How about this? > I think its intent is clearer, plus it's a closer match for the description > in my commit message: > > unsigned long left_redzone_start; > unsigned long object_end; > unsigned long right_redzone_start, right_redzone_end; > > left_redzone_start = addr - KASAN_ALLOCA_REDZONE_SIZE; > kasan_poison_shadow((const void *)left_redzone_start, > KASAN_ALLOCA_REDZONE_SIZE, > KASAN_ALLOCA_LEFT); > > object_end = round_up(addr + size, KASAN_SHADOW_SCALE_SIZE); > right_redzone_start = round_up(object_end, > KASAN_ALLOCA_REDZONE_SIZE); > right_redzone_end = right_redzone_start + KASAN_ALLOCA_REDZONE_SIZE; > kasan_poison_shadow((const void *)object_end, > right_redzone_end - object_end, > KASAN_ALLOCA_RIGHT);