From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751628AbeEDIah (ORCPT ); Fri, 4 May 2018 04:30:37 -0400 Received: from mail-lf0-f65.google.com ([209.85.215.65]:33802 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751060AbeEDIaf (ORCPT ); Fri, 4 May 2018 04:30:35 -0400 X-Google-Smtp-Source: AB8JxZrWgNaoG3uxXR8yYO4hMN6feaOt1TwbeC5cnJUrSR9heSCicqkB13BPn0PeoFcK9gbFCnKvPA== Reply-To: alex.popov@linux.com Subject: Re: [PATCH 2/2] arm64: Clear the stack To: Laura Abbott , Mark Rutland Cc: Kees Cook , Ard Biesheuvel , kernel-hardening@lists.openwall.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <20180502203326.9491-1-labbott@redhat.com> <20180502203326.9491-3-labbott@redhat.com> <20180503071917.xm2xvgagvzkworay@salmiak> <5b654bb4-64cc-dc64-afd7-135971b54c98@redhat.com> From: Alexander Popov Openpgp: preference=signencrypt Autocrypt: addr=alex.popov@linux.com; prefer-encrypt=mutual; keydata= xsFNBFX15q4BEADZartsIW3sQ9R+9TOuCFRIW+RDCoBWNHhqDLu+Tzf2mZevVSF0D5AMJW4f UB1QigxOuGIeSngfmgLspdYe2Kl8+P8qyfrnBcS4hLFyLGjaP7UVGtpUl7CUxz2Hct3yhsPz ID/rnCSd0Q+3thrJTq44b2kIKqM1swt/F2Er5Bl0B4o5WKx4J9k6Dz7bAMjKD8pHZJnScoP4 dzKPhrytN/iWM01eRZRc1TcIdVsRZC3hcVE6OtFoamaYmePDwWTRhmDtWYngbRDVGe3Tl8bT 7BYN7gv7Ikt7Nq2T2TOfXEQqr9CtidxBNsqFEaajbFvpLDpUPw692+4lUbQ7FL0B1WYLvWkG cVysClEyX3VBSMzIG5eTF0Dng9RqItUxpbD317ihKqYL95jk6eK6XyI8wVOCEa1V3MhtvzUo WGZVkwm9eMVZ05GbhzmT7KHBEBbCkihS+TpVxOgzvuV+heCEaaxIDWY/k8u4tgbrVVk+tIVG 99v1//kNLqd5KuwY1Y2/h2MhRrfxqGz+l/f/qghKh+1iptm6McN//1nNaIbzXQ2Ej34jeWDa xAN1C1OANOyV7mYuYPNDl5c9QrbcNGg3D6gOeGeGiMn11NjbjHae3ipH8MkX7/k8pH5q4Lhh Ra0vtJspeg77CS4b7+WC5jlK3UAKoUja3kGgkCrnfNkvKjrkEwARAQABzSZBbGV4YW5kZXIg UG9wb3YgPGFsZXgucG9wb3ZAbGludXguY29tPsLBgAQTAQoAKgIbIwIeAQIXgAULCQgHAwUV CgkICwUWAgMBAAUJB8+UXAUCWgsUegIZAQAKCRCODp3rvH6PqqpOEACX+tXHOgMJ6fGxaNJZ HkKRFR/9AGP1bxp5QS528Sd6w17bMMQ87V5NSFUsTMPMcbIoO73DganKQ3nN6tW0ZvDTKpRt pBUCUP8KPqNvoSs3kkskaQgNQ3FXv46YqPZ7DoYj9HevY9NUyGLwCTEWD2ER5zKuNbI2ek82 j4rwdqXn9kqqBf1ExAoEsszeNHzTKRl2d+bXuGDcOdpnOi7avoQfwi/O0oapR+goxz49Oeov YFf1EVaogHjDBREaqiqJ0MSKexfVBt8RD9ev9SGSIMcwfhgUHhMTX2JY/+6BXnUbzVcHD6HR EgqVGn/0RXfJIYmFsjH0Z6cHy34Vn+aqcGa8faztPnmkA/vNfhw8k5fEE7VlBqdEY8YeOiza hHdpaUi4GofNy/GoHIqpz16UulMjGB5SBzgsYKgCO+faNBrCcBrscWTl1aJfSNJvImuS1JhB EQnl/MIegxyBBRsH68x5BCffERo4FjaG0NDCmZLjXPOgMvl3vRywHLdDZThjAea3pwdGUq+W C77i7tnnUqgK7P9i+nEKwNWZfLpfjYgH5JE/jOgMf4tpHvO6fu4AnOffdz3kOxDyi+zFLVcz rTP5b46aVjI7D0dIDTIaCKUT+PfsLnJmP18x7dU/gR/XDcUaSEbWU3D9u61AvxP47g7tN5+a 5pFIJhJ44JLk6I5H/c7BTQRV9eauARAArcUVf6RdT14hkm0zT5TPc/3BJc6PyAghV/iCoPm8 kbzjKBIK80NvGodDeUV0MnQbX40jjFdSI0m96HNt86FtifQ3nwuW/BtS8dk8+lakRVwuTgMb hJWmXqKMFdVRCbjdyLbZWpdPip0WGND6p5i801xgPRmI8P6e5e4jBO4Cx1ToIFyJOzD/jvtb UhH9t5/naKUGa5BD9gSkguooXVOFvPdvKQKca19S7bb9hzjySh63H4qlbhUrG/7JGhX+Lr3g DwuAGrrFIV0FaVyIPGZ8U2fjLKpcBC7/lZJv0jRFpZ9CjHefILxt7NGxPB9hk2iDt2tE6jSl GNeloDYJUVItFmG+/giza2KrXmDEFKl+/mwfjRI/+PHR8PscWiB7S1zhsVus3DxhbM2mAK4x mmH4k0wNfgClh0Srw9zCU2CKJ6YcuRLi/RAAiyoxBb9wnSuQS5KkxoT32LRNwfyMdwlEtQGp WtC/vBI13XJVabx0Oalx7NtvRCcX1FX9rnKVjSFHX5YJ48heAd0dwRVmzOGL/EGywb1b9Q3O IWe9EFF8tmWV/JHs2thMz492qTHA5pm5JUsHQuZGBhBU+GqdOkdkFvujcNu4w7WyuEITBFAh 5qDiGkvY9FU1OH0fWQqVU/5LHNizzIYN2KjU6529b0VTVGb4e/M0HglwtlWpkpfQzHMAEQEA AcLBZQQYAQIADwUCVfXmrgIbDAUJCWYBgAAKCRCODp3rvH6PqrZtEACKsd/UUtpKmy4mrZwl 053nWp7+WCE+S9ke7CFytmXoMWf1CIrcQTk5cmdBmB4E0l3sr/DgKlJ8UrHTdRLcZZnbVqur +fnmVeQy9lqGkaIZvx/iXVYUqhT3+DNj9Zkjrynbe5pLsrGyxYWfsPRVL6J4mQatChadjuLw 7/WC6PBmWkRA2SxUVpxFEZlirpbboYWLSXk9I3JmS5/iJ+P5kHYiB0YqYkd1twFXXxixv1GB Zi/idvWTK7x6/bUh0AAGTKc5zFhyR4DJRGROGlFTAYM3WDoa9XbrHXsggJDLNoPZJTj9DMww u28SzHLvR3t2pY1dT61jzKNDLoE3pjvzgLKF/Olif0t7+m0IPKY+8umZvUEhJ9CAUcoFPCfG tEbL6t1xrcsT7dsUhZpkIX0Qc77op8GHlfNd/N6wZUt19Vn9G8B6xrH+dinc0ylUc4+4yxt6 6BsiEzma6Ah5jexChYIwaB5Oi21yjc6bBb4l6z01WWJQ052OGaOBzi+tS5iGmc5DWH4/pFqX OIkgJVVgjPv2y41qV66QJJEi2wT4WUKLY1zA9s6KXbt8dVSzJsNFvsrAoFdtzc8v6uqCo0/W f0Id8MBKoqN5FniTHWNxYX6b2dFwq8i5Rh6Oxc6q75Kg8279+co3/tLCkU6pGga28K7tUP2z h9AUWENlnWJX/YhP8MLBZQQYAQoADwIbDAUCWgsSOgUJB9eShwAKCRCODp3rvH6PqtoND/41 ozCKAS4WWBBCU6AYLm2SoJ0EGhg1kIf9VMiqy5PKlSrAnW5yl4WJQcv5wER/7EzvZ49Gj8aG uRWfz3lyQU8dH2KG6KLilDFCZF0mViEo2C7O4QUx5xmbpMUq41fWjY947Xvd3QDisc1T1/7G uNBAALEZdqzwnKsT9G27e9Cd3AW3KsLAD4MhsALFARg6OuuwDCbLl6k5fu++26PEqORGtpJQ rRBWan9ZWb/Y57P126IVIylWiH6vt6iEPlaEHBU8H9+Z0WF6wJ5rNz9gR6GhZhmo1qsyNedD 1HzOsXQhvCinsErpZs99VdZSF3d54dac8ypH4hvbjSmXZjY3Sblhyc6RLYlru5UXJFh7Hy+E TMuCg3hIVbdyFSDkvxVlvhHgUSf8+Uk3Ya4MO4a5l9ElUqxpSqYH7CvuwkG+mH5mN8tK3CCd +aKPCxUFfil62DfTa7YgLovr7sHQB+VMQkNDPXleC+amNqJb423L8M2sfCi9gw/lA1ha6q80 ydgbcFEkNjqz4OtbrSwEHMy/ADsUWksYuzVbw7/pQTc6OAskESBr5igP7B/rIACUgiIjdOVB ktD1IQcezrDcuzVCIpuq8zC6LwLm7V1Tr6zfU9FWwnqzoQeQZH4QlP7MBuOeswCpxIl07mz9 jXz/74kjFsyRgZA+d6a1pGtOwITEBxtxxg== Message-ID: Date: Fri, 4 May 2018 11:30:31 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <5b654bb4-64cc-dc64-afd7-135971b54c98@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03.05.2018 22:09, Laura Abbott wrote: > On 05/03/2018 10:33 AM, Alexander Popov wrote: >> On 03.05.2018 10:19, Mark Rutland wrote: >>> On Wed, May 02, 2018 at 01:33:26PM -0700, Laura Abbott wrote: >>>> + /* Reset the lowest_stack value for the next syscall */ >>>> + current->thread.lowest_stack = current_stack_pointer; >> >> Laura, that might be wrong and introduce huge performance impact. >> >> I think, lowest_stack should be reset similarly to the original version. >> > > Sorry, I'm not understanding here. What's the performance impact and > what do you mean by original version? I meant the code for x86: /* Reset the lowest_stack value for the next syscall */ current->thread.lowest_stack = current_top_of_stack() - 256; ...Now when I'm writing about the performance impact, I see that I was wrong about "huge". Excuse me. Let me describe the implications of this code change. So we are at the end of a syscall. We've just erased the used part of the kernel stack. The current stack pointer is near to the top of stack. On x86_64 I see that the stack pointer is stack top minus 56 bytes (just before switching onto the trampoline stack). I took the idea of resetting lowest_stack to stack top minus 256 from the original PaX Team's code. It should give the speedup when lowest_stack is not updated during a syscall (a lot of functions are not instrumented) and we start to search for the poison value from that reasonable point. If we speak about the common erase_kstack() code, this code change can break x86, because this function can be called from the trampoline stack (separate from the thread stack). >>>> +} >>> >>> Once this function returns, its data is left on the stack. Is that not a problem? >>> >>> No strong feelings either way, but it might be worth mentioning in the commit >>> message. >> >> I managed to bypass that with "register" specifier. Although it doesn't give an >> absolute guarantee. >> > > I guess I was assuming gcc would be smart enough not to spill stuff > on the stack. I also intentionally removed the register keyword > since it wasn't clear gcc does much with it on a modern system? I > could be completely off base here though so please correct me if > I'm wrong. It probably is worth documenting what we are assuming about > the compiler here. I think having register storage class specifier here is a bit better than nothing. And yes, I'll add a comment. Right now don't see a better solution. >>>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile >>>> index a34e9290a699..25dd2a14560d 100644 >>>> --- a/drivers/firmware/efi/libstub/Makefile >>>> +++ b/drivers/firmware/efi/libstub/Makefile >>>> @@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt >>>> KBUILD_CFLAGS := $(cflags-y) -DDISABLE_BRANCH_PROFILING \ >>>> -D__NO_FORTIFY \ >>>> $(call cc-option,-ffreestanding) \ >>>> - $(call cc-option,-fno-stack-protector) >>>> + $(call cc-option,-fno-stack-protector) \ >>>> + $(DISABLE_STACKLEAK_PLUGIN) >>>> >>>> GCOV_PROFILE := n >>>> KASAN_SANITIZE := n >>> >>> I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/. >> >> Could you please give more details on that? Why STACKLEAK breaks it? >> > > For reference, I originally added this for the efistub because > it would not compile. I guess it was a linkage error, right? > I did compile this against my Fedora tree which has KVM enabled. Looked through this big article about ARM, KVM and HYP mode: https://lwn.net/Articles/557132/ So we have some limited amount of kernel code which runs in HYP mode. Is it only in arch/arm64/kvm/hyp/ directory? Mark, could you give a clue what trouble will we have if we call track_stack() or check_alloca() from that code? Thanks in advance! -- Alexander From mboxrd@z Thu Jan 1 00:00:00 1970 From: alex.popov@linux.com (Alexander Popov) Date: Fri, 4 May 2018 11:30:31 +0300 Subject: [PATCH 2/2] arm64: Clear the stack In-Reply-To: <5b654bb4-64cc-dc64-afd7-135971b54c98@redhat.com> References: <20180502203326.9491-1-labbott@redhat.com> <20180502203326.9491-3-labbott@redhat.com> <20180503071917.xm2xvgagvzkworay@salmiak> <5b654bb4-64cc-dc64-afd7-135971b54c98@redhat.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 03.05.2018 22:09, Laura Abbott wrote: > On 05/03/2018 10:33 AM, Alexander Popov wrote: >> On 03.05.2018 10:19, Mark Rutland wrote: >>> On Wed, May 02, 2018 at 01:33:26PM -0700, Laura Abbott wrote: >>>> + /* Reset the lowest_stack value for the next syscall */ >>>> + current->thread.lowest_stack = current_stack_pointer; >> >> Laura, that might be wrong and introduce huge performance impact. >> >> I think, lowest_stack should be reset similarly to the original version. >> > > Sorry, I'm not understanding here. What's the performance impact and > what do you mean by original version? I meant the code for x86: /* Reset the lowest_stack value for the next syscall */ current->thread.lowest_stack = current_top_of_stack() - 256; ...Now when I'm writing about the performance impact, I see that I was wrong about "huge". Excuse me. Let me describe the implications of this code change. So we are at the end of a syscall. We've just erased the used part of the kernel stack. The current stack pointer is near to the top of stack. On x86_64 I see that the stack pointer is stack top minus 56 bytes (just before switching onto the trampoline stack). I took the idea of resetting lowest_stack to stack top minus 256 from the original PaX Team's code. It should give the speedup when lowest_stack is not updated during a syscall (a lot of functions are not instrumented) and we start to search for the poison value from that reasonable point. If we speak about the common erase_kstack() code, this code change can break x86, because this function can be called from the trampoline stack (separate from the thread stack). >>>> +} >>> >>> Once this function returns, its data is left on the stack. Is that not a problem? >>> >>> No strong feelings either way, but it might be worth mentioning in the commit >>> message. >> >> I managed to bypass that with "register" specifier. Although it doesn't give an >> absolute guarantee. >> > > I guess I was assuming gcc would be smart enough not to spill stuff > on the stack. I also intentionally removed the register keyword > since it wasn't clear gcc does much with it on a modern system? I > could be completely off base here though so please correct me if > I'm wrong. It probably is worth documenting what we are assuming about > the compiler here. I think having register storage class specifier here is a bit better than nothing. And yes, I'll add a comment. Right now don't see a better solution. >>>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile >>>> index a34e9290a699..25dd2a14560d 100644 >>>> --- a/drivers/firmware/efi/libstub/Makefile >>>> +++ b/drivers/firmware/efi/libstub/Makefile >>>> @@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt >>>> KBUILD_CFLAGS := $(cflags-y) -DDISABLE_BRANCH_PROFILING \ >>>> -D__NO_FORTIFY \ >>>> $(call cc-option,-ffreestanding) \ >>>> - $(call cc-option,-fno-stack-protector) >>>> + $(call cc-option,-fno-stack-protector) \ >>>> + $(DISABLE_STACKLEAK_PLUGIN) >>>> >>>> GCOV_PROFILE := n >>>> KASAN_SANITIZE := n >>> >>> I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/. >> >> Could you please give more details on that? Why STACKLEAK breaks it? >> > > For reference, I originally added this for the efistub because > it would not compile. I guess it was a linkage error, right? > I did compile this against my Fedora tree which has KVM enabled. Looked through this big article about ARM, KVM and HYP mode: https://lwn.net/Articles/557132/ So we have some limited amount of kernel code which runs in HYP mode. Is it only in arch/arm64/kvm/hyp/ directory? Mark, could you give a clue what trouble will we have if we call track_stack() or check_alloca() from that code? Thanks in advance! -- Alexander