From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f54.google.com ([74.125.83.54]:36810 "EHLO mail-pg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751216AbdGMWkD (ORCPT ); Thu, 13 Jul 2017 18:40:03 -0400 Received: by mail-pg0-f54.google.com with SMTP id u62so36116791pgb.3 for ; Thu, 13 Jul 2017 15:40:03 -0700 (PDT) Subject: Re: [PATCH 1/4] kasan: support alloca() poisoning References: <20170706220114.142438-1-ghackmann@google.com> <20170706220114.142438-2-ghackmann@google.com> From: Greg Hackmann Message-ID: Date: Thu, 13 Jul 2017 15:40:00 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kbuild-owner@vger.kernel.org List-ID: To: Dmitry Vyukov 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 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. 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);