linux-kernel - Re: [PATCH v6 3/6] x86/e820: Refactor range_update and range

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]

Date:   Mon, 7 Feb 2022 13:45:40 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Martin Fernandez <martin.fernandez@...ypsium.com>
Cc:     linux-kernel@...r.kernel.org, linux-efi@...r.kernel.org,
        platform-driver-x86@...r.kernel.org, linux-mm@...ck.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        ardb@...nel.org, dvhart@...radead.org, andy@...radead.org,
        gregkh@...uxfoundation.org, rafael@...nel.org, rppt@...nel.org,
        akpm@...ux-foundation.org, daniel.gutson@...ypsium.com,
        hughsient@...il.com, alex.bazhaniuk@...ypsium.com,
        alison.schofield@...el.com
Subject: Re: [PATCH v6 3/6] x86/e820: Refactor range_update and range_remove

On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote:
> __e820__range_update and e820__range_remove had a very similar
> implementation with a few lines different from each other, the lines
> that actually perform the modification over the e820_table. The
> similiraties were found in the checks for the different cases on how
> each entry intersects with the given range (if it does at all). These
> checks were very presice and error prone so it was not a good idea to
> have them in both places.

Yay removing copy/paste code! :)

> 
> I propose a refactor of those functions, given that I need to create a
> similar one for this patchset.

The diff here is pretty hard (for me) to review; I'll need more time
to check it. What might make review easier (at least for me), is to
incrementally change these routines. i.e. separate patches to:

- add the new infrastructure
- replace e820__range_remove
- replace __e820__range_update

If that's not actually useful, no worries. I'll just stare at it a bit
more. :)

> 
> Add a function to modify a E820 table in a given range. This
> modification is done backed up by two helper structs:
> e820_entry_updater and e820_*_data.
> 
> The first one, e820_entry_updater, carries 3 callbacks which function
> as the actions to take on the table.
> 
> The other one, e820_*_data carries information needed by the
> callbacks, for example in the case of range_update it will carry the
> type that we are targeting.

Something I think would be really amazing here is if you could add KUnit
tests here to exercise the corner cases and validate the changes. It
should be pretty easy to add. Here's a quick example for the boilerplate
and testing a bit of __e820__range_add():

#ifdef CONFIG_E820_KUNIT_TEST
#include <kunit/test.h>

static void __init test_e820_range_add(struct kunit *context)
{
	struct e820_table table;
	u32 full;

	full = ARRAY_SIZE(table.entries);
	/* Add last entry. */
	table->nr_entries = full - 1;
	__e820__range_add(&table, 0, 15, 0);
	KUNIT_EXPECT_EQ(table->nr_entries, full)
	/* Skip new entry when full. */
	__e820__range_add(&table, 0, 15, 0);
	KUNIT_EXPECT_EQ(table->nr_entries, full)
}

static void __init test_e820_update(struct kunit *context)
{
...
}

static struct kunit_case __refdata e820_test_cases[] = {
        KUNIT_CASE(test_e820_range_add),
        KUNIT_CASE(test_e820_update),
	...
        {}
};

static struct kunit_suite e820_test_suite = {
        .name = "e820",
        .test_cases = e820_test_cases,
};

kunit_test_suites(&e820_test_suite);
#endif

-- 
Kees Cook