linux-kernel - Re: [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <704d73f6-30e2-08e0-3a5c-d3639d8b2da1@linux.microsoft.com>
Date:   Thu, 25 Nov 2021 10:59:27 -0600
From:   "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     mark.rutland@....com, jpoimboe@...hat.com, ardb@...nel.org,
        nobuta.keiya@...itsu.com, sjitindarsingh@...il.com,
        catalin.marinas@....com, will@...nel.org, jmorris@...ei.org,
        linux-arm-kernel@...ts.infradead.org,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 4/5] arm64: Introduce stack trace reliability checks
 in the unwinder



On 11/25/21 8:56 AM, Mark Brown wrote:
> On Tue, Nov 23, 2021 at 01:37:22PM -0600, madvenka@...ux.microsoft.com wrote:
> 
>> Introduce arch_stack_walk_reliable() for ARM64. This works like
>> arch_stack_walk() except that it returns -EINVAL if the stack trace is not
>> reliable.
> 
>> Until all the reliability checks are in place, arch_stack_walk_reliable()
>> may not be used by livepatch. But it may be used by debug and test code.
> 
> Probably also worth noting that this doesn't select
> HAVE_RELIABLE_STACKTRACE which is what any actual users are going to use
> to identify if the architecture has the feature.  I would have been
> tempted to add arch_stack_walk() as a separate patch but equally having
> the user code there (even if it itself can't yet be used...) helps with
> reviewing the actual unwinder so I don't mind.
> 

I did not select HAVE_RELIABLE_STACKTRACE just in case we think that some
more reliability checks need to be added. But if reviewers agree
that this patch series contains all the reliability checks we need, I
will add a patch to select HAVE_RELIABLE_STACKTRACE to the series.

>> +static void unwind_check_reliability(struct task_struct *task,
>> +				     struct stackframe *frame)
>> +{
>> +	if (frame->fp == (unsigned long)task_pt_regs(task)->stackframe) {
>> +		/* Final frame; no more unwind, no need to check reliability */
>> +		return;
>> +	}
> 
> If the unwinder carries on for some reason (the code for that is
> elsewhere and may be updated separately...) then this will start
> checking again.  I'm not sure if this is a *problem* as such but the
> thing about this being the final frame coupled with not actually
> explicitly stopping the unwind here makes me think this should at least
> be clearer, the comment begs the question about what happens if
> something decides it is not in fact the final frame.
> 

I can address this by adding an explicit comment to that effect.
For example, define a separate function to check for the final frame:

/*
 * Check if this is the final frame. Unwind must stop at the final
 * frame.
 */
static inline bool unwind_is_final_frame(struct task_struct *task,
                                         struct stackframe *frame)
{
	return frame->fp == (unsigned long)task_pt_regs(task)->stackframe;
}

Then, use this function in unwind_check_reliability() and unwind_continue().

Is this acceptable?

Madhavan