Messages in this thread | | | Date | Tue, 10 Oct 2023 13:41:19 -0700 | From | Josh Poimboeuf <> | Subject | Re: [PATCH 3/3] x86/retpoline: Ensure default return thunk isn't used at runtime |
| |
On Tue, Oct 10, 2023 at 08:14:33PM +0000, Kaplan, David wrote: > [AMD Official Use Only - General] > > > -----Original Message----- > > From: Josh Poimboeuf <jpoimboe@kernel.org> > > Sent: Tuesday, October 10, 2023 2:37 PM > > To: Kaplan, David <David.Kaplan@amd.com> > > Cc: x86@kernel.org; luto@kernel.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH 3/3] x86/retpoline: Ensure default return thunk isn't used > > at runtime > > > > > * > > > - * This code is only used during kernel boot or module init. All > > > + * This code is only used during kernel boot. All > > > * 'JMP __x86_return_thunk' sites are changed to something else by > > > * apply_returns(). > > > + * > > > + * This thunk is turned into a ud2 to ensure it is never used at runtime. > > > + * Alternative instructions are applied after apply_returns(). > > > */ > > > SYM_CODE_START(__x86_return_thunk) > > > UNWIND_HINT_FUNC > > > ANNOTATE_NOENDBR > > > - ANNOTATE_UNRET_SAFE > > > - ret > > > + ALTERNATIVE __stringify(ANNOTATE_UNRET_SAFE;ret),"ud2", > > > + X86_FEATURE_RETHUNK > > > > If it's truly never used after boot (even for non-rethunk cases) then can we use > > X86_FEATURE_ALWAYS? > > > > I think that could work. There is one subtlety though I'll point out: > > The use of __x86_return_thunk when X86_FEATURE_RETHUNK is set is a > potential security issue, as it means the required return thunk is not > being used. The use of __x86_return_thunk when X86_FEATURE_RETHUNK is > not set is only a performance issue, as it means there is a return > that was not rewritten to be an inline 'ret' by apply_returns(). > > The ud2 was primarily intended to capture cases where there is a > potential security hole, while it is a bit overkill just to point out > a return that was not optimized.
Even if it's not a security hole, I'd still view it as a major BUG() as it would directly contradict our understanding (and the comments above) and could cause performance or other correctness issues that would otherwise go unnoticed.
So I think an unconditional UD2 is warranted.
-- Josh
| |