linux-kernel - Re: [PATCH v24 08/24] x86/sgx: Enumerate and track EPC sections

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:   Wed, 18 Dec 2019 10:18:56 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-sgx@...r.kernel.org, akpm@...ux-foundation.org,
        dave.hansen@...el.com, sean.j.christopherson@...el.com,
        nhorman@...hat.com, npmccallum@...hat.com, serge.ayoun@...el.com,
        shay.katz-zamir@...el.com, haitao.huang@...el.com,
        andriy.shevchenko@...ux.intel.com, tglx@...utronix.de,
        kai.svahn@...el.com, josh@...htriplett.org, luto@...nel.org,
        kai.huang@...el.com, rientjes@...gle.com, cedric.xing@...el.com,
        puiterwijk@...hat.com
Subject: Re: [PATCH v24 08/24] x86/sgx: Enumerate and track EPC sections

On Sat, Nov 30, 2019 at 01:13:10AM +0200, Jarkko Sakkinen wrote:
> +static bool __init sgx_alloc_epc_section(u64 addr, u64 size,
> +					 unsigned long index,
> +					 struct sgx_epc_section *section)
> +{
> +	unsigned long nr_pages = size >> PAGE_SHIFT;

I'm assuming here that size which gets communicated through CPUID -
which is an interesting way to communicate SGX settings in itself :-) - is
in multiples of 4K? SDM doesn't say...

And last time I asked:

"This size comes from CPUID but it might be prudent to sanity-check it
nevertheless, before doing the memremap()."

but it was left uncommented.

> +/**
> + * A section metric is concatenated in a way that @low bits 12-31 define the
> + * bits 12-31 of the metric and @high bits 0-19 define the bits 32-51 of the
> + * metric.
> + */
> +static inline u64 __init sgx_calc_section_metric(u64 low, u64 high)
> +{
> +	return (low & GENMASK_ULL(31, 12)) +
> +	       ((high & GENMASK_ULL(19, 0)) << 32);
> +}
> +
> +static bool __init sgx_page_cache_init(void)
> +{
> +	u32 eax, ebx, ecx, edx, type;
> +	u64 pa, size;
> +	int i;
> +
> +	BUILD_BUG_ON(SGX_MAX_EPC_SECTIONS > (SGX_EPC_SECTION_MASK + 1));
> +
> +	for (i = 0; i < (SGX_MAX_EPC_SECTIONS + 1); i++) {

Those brackets are still here from the last time. You said:

"For nothing :-)

I'll change it as:

  for (i = 0; i <= SGX_MAX_EPC_SECTIONS; i++) {"

but probably forgot...

and looking at my review comments here:

https://lkml.kernel.org/r/20191005092627.GA25699@zn.tnic

and your reply:

https://lkml.kernel.org/r/20191007115850.GA20830@linux.intel.com

you clearly missed addressing some so I'm going to stop reviewing here.

Please have a look at those review comments again and check whether the
apply - and then do them - or they don't and they pls explain why they
don't.

And do that for the rest of the patchset, please, before you send it
again.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette