linux-kernel - Re: [PATCH v4] x86/sgx: Free backing memory after faulting the enclave page

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: <Yh39kuf8kuZJ0pRJ@iki.fi>
Date:   Tue, 1 Mar 2022 12:03:46 +0100
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     linux-sgx@...r.kernel.org,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        stable@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Jethro Beekman <jethro@...tanix.com>,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] x86/sgx: Free backing memory after faulting the
 enclave page

On Mon, Feb 28, 2022 at 07:32:02AM -0800, Dave Hansen wrote:
> On 2/28/22 04:55, Jarkko Sakkinen wrote:
> > I thought that the formula is so simple that it does not matter if it is
> > just in two sites open coded but I can wrap it too, if required, e.g.
> > 
> > /* 
> >  * Calculate byte offset of a PCMD struct associated to an enclave page.
> >  * PCMD's follow right after the EPC data in the backing storage. In
> >  * addition to the visible enclave pages, there's one extra page slot
> >  * for SECS, before PCMD data.
> >  */
> > static pgoff_t *sgx_encl_page_index_to_pcmd_offset(struct sgx_encl *encl, unsigned long page_index)
> > {
> >         return encl->size + PAGE_SIZE + page_index * sizeof(struct sgx_pcmd);
> > }
> 
> Yes, it's required.  Please wrap it.
> 
> There's also nothing wrong with spreading that calculation across
> several lines.  It may be arithmetically simple, but it's combining
> three or four logical steps.  There's no shame in separating and
> commenting some of those separately.

I can do that but one thing that would make this more documentative would
be to describe the formula as "encl->size + sizeof(struct sgx_secs) +
sizeof(struct sgx_pcmd)", i.e. PAGE_SIZE is a magic number so the
end result would be:

/* 
 * Calculate byte offset of a PCMD struct associated to an enclave page. PCMD's
 * follow right after the EPC data in the backing storage. In addition to the
 * visible enclave pages, there's one extra page slot for SECS, before PCMD
 * structs.
 */
static inline pgoff_t sgx_encl_page_index_to_pcmd_offset(struct sgx_encl *encl, unsigned long page_index)
{
        pgoff_t epc_end_off = encl->size + sizeof(struct sgx_secs);

        return epc_end_off + page_index * sizeof(struct sgx_pcmd);
}

Now it is hard to get this wrong and also the file offset has the nice quality
of packing the page index for PCMD page, and offset within that page. IMHO,
this will nice and clean long-term way to sort this out.

/Jarkko