linux-kernel - Re: [PATCH 05/25] x86/sgx: Introduce runtime protection bits

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: <Yavw05Op0PTm3AuX@iki.fi>
Date:   Sun, 5 Dec 2021 00:50:59 +0200
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Reinette Chatre <reinette.chatre@...el.com>
Cc:     dave.hansen@...ux.intel.com, tglx@...utronix.de, bp@...en8.de,
        luto@...nel.org, mingo@...hat.com, linux-sgx@...r.kernel.org,
        x86@...nel.org, seanjc@...gle.com, kai.huang@...el.com,
        cathy.zhang@...el.com, cedric.xing@...el.com,
        haitao.huang@...el.com, mark.shanahan@...el.com, hpa@...or.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/25] x86/sgx: Introduce runtime protection bits

What about:

"x86/sgx: Add encl_page->vm_run_prot_bits for dynamic permission changes"

On Wed, Dec 01, 2021 at 11:23:03AM -0800, Reinette Chatre wrote:
> Enclave creators declare their paging permission intent at the time
> the pages are added to the enclave. These paging permissions are
> vetted when pages are added to the enclave and stashed off
> (in sgx_encl_page->vm_max_prot_bits) for later comparison with
> enclave PTEs.
> 
> Current permission support assume that enclave page permissions
> remain static for the lifetime of the enclave. This is about to change
> with the addition of support for SGX2 where the permissions of enclave
> pages belonging to an initialized enclave may be changed during the
> enclave's lifetime.
> 
> Introduce runtime protection bits in preparation for support of

By writing "Introduce runtime protection bits", instead of simply "Add
encl_page->vm_run_prot_bits", the only thing you are adding is obfuscation.

Try to refer to the "exact thing", instead of English rephrasing
whenever possible.

> enclave page permission changes. These bits reflect the active
> permissions of an enclave page and are not to exceed the maximum
> protection bits that passed scrutiny during enclave creation.
> 
> Associate runtime protection bits with each enclave page. Initialize
> the runtime protection bits to the vetted maximum protection bits
> on page creation. Use the runtime protection bits for any access
> checks.

I guess the first sentence in this paragraph is completely redundant
as the first sentence of the previous paragraph tells the exact
same story.

> struct sgx_encl_page hosting this information is maintained for each
> enclave page so the space consumed by the struct is important.
> The existing vm_max_prot_bits is already unsigned long while only using
> three bits. Transition to a bitfield for the two members containing
> protection bits.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@...el.com>

So this commit message left the most important thing unanswered,
or I missed it (which happens quite often): why two fields instead
of one? Why vm_max_port_bits needs to stay constant?

It's something that should be clearly documented.

/Jarkko