linux-kernel - Re: [PATCH RFC 01/10] mm: add Kernel Electric-Fence infrastructure

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] [day] [month] [year] [list]

Message-ID: <20200915142631.31234-1-sjpark@amazon.com>
Date:   Tue, 15 Sep 2020 16:26:31 +0200
From:   SeongJae Park <sjpark@...zon.com>
To:     Marco Elver <elver@...gle.com>
CC:     SeongJae Park <sjpark@...zon.com>, <mark.rutland@....com>,
        <linux-doc@...r.kernel.org>, <peterz@...radead.org>,
        <catalin.marinas@....com>, <dave.hansen@...ux.intel.com>,
        <linux-mm@...ck.org>, <edumazet@...gle.com>, <glider@...gle.com>,
        <hpa@...or.com>, <cl@...ux.com>, <will@...nel.org>,
        <corbet@....net>, <x86@...nel.org>, <kasan-dev@...glegroups.com>,
        <mingo@...hat.com>, <dvyukov@...gle.com>, <rientjes@...gle.com>,
        <aryabinin@...tuozzo.com>, <keescook@...omium.org>,
        <paulmck@...nel.org>, <jannh@...gle.com>, <andreyknvl@...gle.com>,
        <cai@....pw>, <luto@...nel.org>, <tglx@...utronix.de>,
        <akpm@...ux-foundation.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <gregkh@...uxfoundation.org>, <linux-kernel@...r.kernel.org>,
        <penberg@...nel.org>, <bp@...en8.de>, <iamjoonsoo.kim@....com>
Subject: Re: [PATCH RFC 01/10] mm: add Kernel Electric-Fence infrastructure

On Tue, 15 Sep 2020 16:14:49 +0200 Marco Elver <elver@...gle.com> wrote:

> On Tue, Sep 15, 2020 at 03:57PM +0200, SeongJae Park wrote:
> [...]
> > 
> > So interesting feature!  I left some tirvial comments below.
> 
> Thank you!
[...]
> > > +
> > > +	/* Only call with a pointer into kfence_metadata. */
> > > +	if (KFENCE_WARN_ON(meta < kfence_metadata ||
> > > +			   meta >= kfence_metadata + ARRAY_SIZE(kfence_metadata)))
> > 
> > Is there a reason to use ARRAY_SIZE(kfence_metadata) instead of
> > CONFIG_KFENCE_NUM_OBJECTS?
> 
> They're equivalent. We can switch it. (Although I don't see one being
> superior to the other.. maybe we save on compile-time?)

I prefer CONFIG_KFENCE_NUM_OBJECTS here just because it's more widely used in
the code.  Also, I personally think it's more easy to read.

[...]
> > > +	pr_info("initialized - using %zu bytes for %d objects", KFENCE_POOL_SIZE,
> > > +		CONFIG_KFENCE_NUM_OBJECTS);
> > > +	if (IS_ENABLED(CONFIG_DEBUG_KERNEL))
> > > +		pr_cont(" at 0x%px-0x%px\n", (void *)__kfence_pool,
> > > +			(void *)(__kfence_pool + KFENCE_POOL_SIZE));
> > 
> > Why don't you use PTR_FMT that defined in 'kfence.h'?
> 
> It's unnecessary, since all this is conditional on
> IS_ENABLED(CONFIG_DEBUG_KERNEL)) and we can just avoid the indirection
> through PTR_FMT.

Ok, agreed.

[...]
> > > +	for (skipnr = 0; skipnr < num_entries; skipnr++) {
> > > +		int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
> > > +
> > > +		/* Depending on error type, find different stack entries. */
> > > +		switch (type) {
> > > +		case KFENCE_ERROR_UAF:
> > > +		case KFENCE_ERROR_OOB:
> > > +		case KFENCE_ERROR_INVALID:
> > > +			if (!strncmp(buf, KFENCE_SKIP_ARCH_FAULT_HANDLER, len))
> > 
> > Seems KFENCE_SKIP_ARCH_FAULT_HANDLER not defined yet?
> 
> Correct, it'll be defined in <asm/kfence.h> in the x86 and arm64
> patches. Leaving this is fine, since no architecture has selected
> HAVE_ARCH_KFENCE in this patch yet; as a result, we also can't break the
> build even if this is undefined.

Ah, got it.  Thank you for the kind explanation.


Thanks,
SeongJae Park

> 
> Thanks,
> -- Marco