linux-kernel - Re: [PATCH] kfence: Use pt_regs to generate stack trace on faults

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: <20201105105241.GC82102@C02TD0UTHF1T.local>
Date:   Thu, 5 Nov 2020 10:52:41 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Marco Elver <elver@...gle.com>
Cc:     akpm@...ux-foundation.org, glider@...gle.com, dvyukov@...gle.com,
        jannh@...gle.com, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        kasan-dev@...glegroups.com, x86@...nel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] kfence: Use pt_regs to generate stack trace on faults

On Thu, Nov 05, 2020 at 10:21:33AM +0100, Marco Elver wrote:
> Instead of removing the fault handling portion of the stack trace based
> on the fault handler's name, just use struct pt_regs directly.
> 
> Change kfence_handle_page_fault() to take a struct pt_regs, and plumb it
> through to kfence_report_error() for out-of-bounds, use-after-free, or
> invalid access errors, where pt_regs is used to generate the stack
> trace.
> 
> If the kernel is a DEBUG_KERNEL, also show registers for more
> information.
> 
> Suggested-by: Mark Rutland <mark.rutland@....com>
> Signed-off-by: Marco Elver <elver@...gle.com>

Wow; I wasn't expecting this to be put together so quickly, thanks for
doing this!

>From a scan, this looks good to me -- just one question below.

> diff --git a/include/linux/kfence.h b/include/linux/kfence.h
> index ed2d48acdafe..98a97f9d43cd 100644
> --- a/include/linux/kfence.h
> +++ b/include/linux/kfence.h
> @@ -171,6 +171,7 @@ static __always_inline __must_check bool kfence_free(void *addr)
>  /**
>   * kfence_handle_page_fault() - perform page fault handling for KFENCE pages
>   * @addr: faulting address
> + * @regs: current struct pt_regs (can be NULL, but shows full stack trace)
>   *
>   * Return:
>   * * false - address outside KFENCE pool,

> @@ -44,8 +44,12 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
>  		case KFENCE_ERROR_UAF:
>  		case KFENCE_ERROR_OOB:
>  		case KFENCE_ERROR_INVALID:
> -			is_access_fault = true;
> -			break;
> +			/*
> +			 * kfence_handle_page_fault() may be called with pt_regs
> +			 * set to NULL; in that case we'll simply show the full
> +			 * stack trace.
> +			 */
> +			return 0;

For both the above comments, when/where is kfence_handle_page_fault()
called with regs set to NULL? I couldn't spot that in this patch, so
unless I mised it I'm guessing that's somewhere outside of the patch
context?

If this is a case we don't expect to happen, maybe add a WARN_ON_ONCE()?

Thanks,
Mark.