Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Dec 2022 12:11:19 +0000
From:   Will Deacon <will@kernel.org>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     catalin.marinas@arm.com, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, kernel-team@android.com,
        maz@kernel.org, ardb@kernel.org
Subject: Re: [GIT PULL] arm64 updates for 6.2
Message-ID: <20221213121118.GB5719@willie-the-truck>
References: <20221209112500.GA3116@willie-the-truck>
 <CAHk-=wj-8gKB5SG0w_M2fK0KYyhZbNzDZ1W9H5pmisWzJ0XJEw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wj-8gKB5SG0w_M2fK0KYyhZbNzDZ1W9H5pmisWzJ0XJEw@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hi Linus,

[+Ard]

On Mon, Dec 12, 2022 at 10:05:07AM -0800, Linus Torvalds wrote:
> On Fri, Dec 9, 2022 at 3:25 AM Will Deacon <will@kernel.org> wrote:
> >
> > Dynamic SCS:
> >         * Support for dynamic shadow call stacks to allow switching at
> >           runtime between Clang's SCS implementation and the CPU's
> >           pointer authentication feature when it is supported (complete
> >           with scary DWARF parser!)
> 
> I've pulled this thing, but this part makes me nervous. There's some
> bad history with debug information not being 100% reliable probably
> simply because it gets very little correctness testing.

Hey, I did use the word "scary"! This is, at least, very easy to back
out (it's effectively an optimisation) if the DWARF info ends up being
too unreliable and causes issues in practice. We're also only looking
at .eh_frame here, which should hopefully get a lot more correctness
testing when compared to the .debug sections due to exception unwinding.

> It might be worth thinking about at least verifying the information
> using something like objtool, so that you at least catch problem cases
> at *build* time rather than runtime.

Checking that the DWARF data looks sensible at build time isn't a bad
idea, but see below as I think we can probably still produce a functional
kernel Image in this case.

> For example, that whole
> 
>     default:
>         pr_err("unhandled opcode: %02x in FDE frame %lx\n",
> opcode[-1], (uintptr_t)frame);
>         return -ENOEXEC;
> 
> really makes me go "this should have been verified at build time, it's
> much too late to notice now that you don't understand the dwarf data".

This isn't actually as bad as it looks -- the patching operation here
only kicks in on CPUs which do not implement the pointer authentication
instructions (i.e. where the CPU executes these as NOPs). Therefore, if
patching bails out half way due to the "unhandled opcode" above, we
should be ok, albeit missing some SCS coverage. I say "should" because
if we fail within a frame after patching in the SCS "push" but before
patching in the "pop", then we'd end up with a corrupt SCS pointer.

Ard -- do you think we could tweak the patching so that we patch the push
and the pop together (e.g. by tracking the two locations on a per-frame
basis and postponing the text poking until just before we return from
scs_handle_fde_frame())?

Will