From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753115AbcHORW5 (ORCPT ); Mon, 15 Aug 2016 13:22:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34108 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753059AbcHORW4 (ORCPT ); Mon, 15 Aug 2016 13:22:56 -0400 Date: Mon, 15 Aug 2016 12:22:52 -0500 From: Josh Poimboeuf To: Andy Lutomirski Cc: Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , X86 ML , "linux-kernel@vger.kernel.org" , Linus Torvalds , Steven Rostedt , Brian Gerst , Kees Cook , Peter Zijlstra , Frederic Weisbecker , Byungchul Park , Nilay Vaish Subject: Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Message-ID: <20160815172252.k26t7jd7ssnxkcsr@treble> References: <739031d9151dbd300f3c07035bb239a2ac0e8b02.1471011425.git.jpoimboe@redhat.com> <20160815150558.ivur3rpadnk3nz7x@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160815150558.ivur3rpadnk3nz7x@treble> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Mon, 15 Aug 2016 17:22:55 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 15, 2016 at 10:05:58AM -0500, Josh Poimboeuf wrote: > On Sun, Aug 14, 2016 at 12:26:29AM -0700, Andy Lutomirski wrote: > > On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf wrote: > > > On x86_32, when an interrupt happens from kernel space, SS and SP aren't > > > pushed and the existing stack is used. So pt_regs is effectively two > > > words shorter, and the previous stack pointer is normally the memory > > > after the shortened pt_regs, aka '®s->sp'. > > > > > > But in the rare case where the interrupt hits right after the stack > > > pointer has been changed to point to an empty stack, like for example > > > when call_on_stack() is used, the address immediately after the > > > shortened pt_regs is no longer on the stack. In that case, instead of > > > '®s->sp', the previous stack pointer should be retrieved from the > > > beginning of the current stack page. > > > > > > kernel_stack_pointer() wants to do that, but it forgets to dereference > > > the pointer. So instead of returning a pointer to the previous stack, > > > it returns a pointer to the beginning of the current stack. > > > > > > Fixes: 0788aa6a23cb ("x86: Prepare removal of previous_esp from i386 thread_info structure") > > > Signed-off-by: Josh Poimboeuf > > > > This seems like a valid fix, but I'm not sure I agree with the intent > > of the code. ®s->sp really is the previous stack pointer in the > > sense that the stack pointer was ®s->sp when the entry happened. > > From an unwinder's perspective, how is: > > > > movl [whatever], $esp > > <-- interrupt > > > > any different from: > > > > movl [whatever], $esp > > pushl [something] > > <-- interrupt > > In the first case, the stack is empty, so reading the value pointed to > by %esp would result in accessing outside the bounds of the stack. ...but maybe your point is that following the previous stack pointer is outside the scope of kernel_stack_pointer() and should instead be done by its caller. Especially considering the fact that the x86_64 version of this function doesn't have this "feature". In which case I think I would agree. However I think fixing that is outside the scope of this already-way-too-big patch set. -- Josh