Date:   Mon, 7 Nov 2022 10:28:13 +0800
From:   Aaron Lu <aaron.lu@...el.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     Dave Hansen <dave.hansen@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        "Yin, Fengwei" <fengwei.yin@...el.com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        <linux-kselftest@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2] selftest/x86/meltdown: Add a selftest for meltdown

Hi Greg,

Thanks for taking a look.

On Sun, Nov 06, 2022 at 12:27:12PM +0100, Greg KH wrote:
> On Sun, Nov 06, 2022 at 06:19:39PM +0800, Aaron Lu wrote:
> > To capture potential programming errors like mistakenly setting Global
> > bit on kernel page table entries, a selftest for meltdown is added.
> > 
> > This selftest is based on https://github.com/IAIK/meltdown. What this
> > test does is to firstly set a predefined string at a random user address
> > and then with pagemap, get the physical address of this string. Finally,
> > try to fetch the data using kernel's directmap address for this physical
> > address to see if user space can use kernel's page table.
> 
> As this is based on someone else's code, what happened to the proper
> credit for them as the author and copyright owner?

Should I list the contributors in the patch header comment section?
Something like this:

The original code is contributed by:
$ git shortlog -sne --all
    24  Michael Schwarz <michael.schwarz91@...il.com>
    23  Michael Schwarz <michael.schwarz@...dent.tugraz.at>
     9  Pavel Boldin <boldin.pavel@...il.com>
     6  Daniel Gruss <lava@...ss.cc>
     3  Daniel Gruss <daniel.gruss@...k.tugraz.at>
     3  Jared Deckard <jdeckard@...ityins.net>
     3  Moritz Lipp <github@....me>
     2  Matteo Croce <mcroce@...hat.com>
     2  Raphael Carvalho <raphael.scarv@...il.com>
     2  asgh <asgh@...rs.noreply.github.com>
     1  Eduardo Marques <eduardorbmarques@...il.com>
     1  Egor Vorontsov <sdoregor@...re.me>
     1  Jakub Wilk <jwilk@...lk.net>
     1  Jason Davies <jason@...ondavies.com>
     1  Lukasz Gryglicki <lukaszgryglicki@...pl>
     1  Michael Schwarz <michael.schwarz@...k.tugraz.at>
     1  Raphael S. Carvalho <raphaelsc@...lladb.com>
     1  Steven <steven@...iously.com>
     1  Vamsi Krishna <vamsi3@...look.com>
     1  pierwill <19642016+pierwill@...rs.noreply.github.com>
     1  ysiyer <yegnesh.s.iyer@...el.com>

As for copyright, the only copyright I can find in the referenced repo
is in the LICENSE file and it is: Copyright (c) 2018 meltdown, I'm not
sure if I'm allowed to add copyright statement for others.

> > Per my tests, this test works well on CPUs that have TSX support. For
> > this reason, this selftest only works on CPUs that supports TSX.
> > 
> > This test requires the knowledge of direct map base. IAIK used the
> > following two methods to get direct map base:
> > 1 through a kernel module to show phys_to_virt(0);
> > 2 by exploiting the same HW vulnerability to guess the base.
> > Method 1 makes running this selftest complex while method 2 is not
> > reliable and I do not want to use a possibly wrong value to run this
> > test. Suggestions are welcome.
> > 
> > Tested on both x86_64 and i386_pae VMs on a host with i7-7700K cpu,
> > success rate is about 50% when nopti kernel cmdline is used.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@...el.com>
> > ---
> > v2:
> > - Added [SKIP], [INFO] prefix to some prints;
> > - Do not run 32bits test on 64bits host since it doesn't make sense to
> >   do that;
> > - Minor comment update.
> > 
> >  tools/testing/selftests/x86/Makefile   |   2 +-
> >  tools/testing/selftests/x86/meltdown.c | 418 +++++++++++++++++++++++++
> >  2 files changed, 419 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/x86/meltdown.c
> > 
> > diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
> > index 0388c4d60af0..36f99c360a56 100644
> > --- a/tools/testing/selftests/x86/Makefile
> > +++ b/tools/testing/selftests/x86/Makefile
> > @@ -13,7 +13,7 @@ CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh "$(CC)" trivial_program.c -no-pie)
> >  TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt test_mremap_vdso \
> >  			check_initial_reg_state sigreturn iopl ioperm \
> >  			test_vsyscall mov_ss_trap \
> > -			syscall_arg_fault fsgsbase_restore sigaltstack
> > +			syscall_arg_fault fsgsbase_restore sigaltstack meltdown
> >  TARGETS_C_32BIT_ONLY := entry_from_vm86 test_syscall_vdso unwind_vdso \
> >  			test_FCMOV test_FCOMI test_FISTTP \
> >  			vdso_restorer
> > diff --git a/tools/testing/selftests/x86/meltdown.c b/tools/testing/selftests/x86/meltdown.c
> > new file mode 100644
> > index 000000000000..8c0c1db49096
> > --- /dev/null
> > +++ b/tools/testing/selftests/x86/meltdown.c
> > @@ -0,0 +1,418 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> 
> That's nice but that does NOT reflect the license of the code below, as
> you state in the comments.
> 
> You might want to get an Intel lawyer to look this over and give you
> advice on how to properly tag this license.

Thanks for the advice.

I've gone through our internal process and was told it is OK to
relicense zlib licensed code to GPLv2.

> 
> > +/*
> > + * This selftest is based on code from https://github.com/IAIK/meltdown
> > + * and can be used to check if user space can read data through kernel
> > + * page table entries.
> > + *
> > + * Note for i386 test: due to kernel prefer to use high memory for user
> > + * programs, it is necessary to restrict the available memory under that
> > + * of low memory size(around ~896MiB) so that the memory hosting "string"
> > + * in main() is directly mapped.
> > + *
> > + * Note for both x86_64 and i386 test: the hardware race window can not be
> > + * exploited 100% each time so a single run of the test on a vulnerable system
> > + * may not FAIL. My tests on a i7-7700K cpu have a success rate about 50%.
> > + *
> > + * The original copyright and license information are shown below:
> > + *
> > + * Copyright (c) 2018 meltdown
> 
> I don't see that copyright in the original github repo, are you sure
> about that?  I see individual developers contributing there instead.
> Please keep authorship information when you know it.

There is a license file in this repo and I've kept the content intact
in the patch, please see this:
https://github.com/IAIK/meltdown/blob/master/LICENSE

Thanks,
Aaron

Powered by blists - more mailing lists