Re: [PATCH 2/9] ARM: traps: use get_kernel_nofault instead of set_fs() - Russell King

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Christoph Hellwig <hch@lst.de>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux- <kernel@vger.kernel.org>,
	linux-arch <linux-arch@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Dmitry Safonov <0x7f454c46@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/9] ARM: traps: use get_kernel_nofault instead of set_fs()
Date: Fri, 18 Sep 2020 08:42:03 +0100	[thread overview]
Message-ID: <20200918074203.GU1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <CAK8P3a22EiD-uMZQaBpHQYyy=MJ_7J-ih=6CtgH_9RXT6OOYvg@mail.gmail.com>

On Thu, Sep 17, 2020 at 07:29:37PM +0200, Arnd Bergmann wrote:
> On Tue, Sep 8, 2020 at 8:15 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > > +static void dump_mem(const char *, const char *, unsigned long, unsigned long, bool kernel_mode);
> >
> > This adds a pointlessly long line.
> 
> Fixed.
> 
> > And looking at the code I don't see why the argument is even needed.
> >
> > dump_mem() currently does an unconditional set_fs(KERNEL_DS), so it
> > should always use get_kernel_nofault.
> 
> I had looked at
> 
>         if (!user_mode(regs) || in_interrupt()) {
>                 dump_mem(KERN_EMERG, "Stack: ", regs->ARM_sp,
>                          THREAD_SIZE + (unsigned
> long)task_stack_page(tsk), kernel_mode);
> 
> which told me that there should be at least some code path ending up in
> __die() that has in_interrupt() set but comes from user mode.
> 
> In this case, the memory to print would be a user pointer and cannot be
> accessed by get_kernel_nofault() (but could be accessed with
> "set_fs(KERNEL_DS); __get_user()").
> 
> I looked through the history now and the only code path I could
> find that would arrive here this way is from bad_mode(), indicating
> that there is probably a hardware bug or the contents of *regs are
> corrupted.

Yes, that's correct.  It isn't something entirely theoretical, although
we never see it now, it used to happen in the distant past due to saved
regs corruption.  If bad_mode() ever gets called, all bets are off and
we're irrecoverably crashing.

Note that in that case, while user_mode(regs) may return true or false,
regs->ARM_sp and regs->ARM_lr are always the SVC mode stack and return
address after regs has been stacked, and not the expected values for
the parent context (which we have most likely long since destroyed.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!