linux-kernel - Re: [PATCHv4 3/8] efi/x86: Implement support for unaccepted memory

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: <20220418235015.mnujtlmmlyin7y6m@box.shutemov.name>
Date:   Tue, 19 Apr 2022 02:50:15 +0300
From:   "Kirill A. Shutemov" <kirill@...temov.name>
To:     Borislav Petkov <bp@...en8.de>
Cc:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Sean Christopherson <seanjc@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Joerg Roedel <jroedel@...e.de>,
        Ard Biesheuvel <ardb@...nel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Tom Lendacky <thomas.lendacky@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        Varad Gautam <varad.gautam@...e.com>,
        Dario Faggioli <dfaggioli@...e.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Brijesh Singh <brijesh.singh@....com>,
        Mike Rapoport <rppt@...nel.org>,
        David Hildenbrand <david@...hat.com>, x86@...nel.org,
        linux-mm@...ck.org, linux-coco@...ts.linux.dev,
        linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv4 3/8] efi/x86: Implement support for unaccepted memory

On Mon, Apr 18, 2022 at 11:01:12PM +0200, Borislav Petkov wrote:
> On Mon, Apr 18, 2022 at 11:24:31PM +0300, Kirill A. Shutemov wrote:
> > <linux/bitmap> doesn't include <linux/kernel.h> or similar things.
> > Is it okay for now?
> 
> No, it is not ok because those linux/ includes are moving targets. They
> keep changing and then that indirectly influences the decompressor.
> 
> How much functionality from linux/bitmap.h do you actually need?

Below is the bare minimum required to compile bitmap.c in decompresser.
I only made it work on my config/compiler and did not care about all
#ifdef branches.

I find it strange that you go after <linux/bitmap.h> which has limited
exposure while <linux/acpi.h> and <linux/efi.h> are there already.
Starting small will backfire if once we find out that monstrous headers
depend on what we try to replace. Bit fish has to be addressed first.

What do you want me to do here?

// <linux/bitmap.h>
//
#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
#define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))

// <uapi/linux/swab.>

/**
 * __swab64 - return a byteswapped 64-bit value
 * @x: value to byteswap
 */
#ifdef __HAVE_BUILTIN_BSWAP64__
#define __swab64(x) (__u64)__builtin_bswap64((__u64)(x))
#else
#define __swab64(x)				\
	(__builtin_constant_p((__u64)(x)) ?	\
	___constant_swab64(x) :			\
	__fswab64(x))
#endif

static __always_inline unsigned long __swab(const unsigned long y)
{
#if __BITS_PER_LONG == 64
	return __swab64(y);
#else /* __BITS_PER_LONG == 32 */
	return __swab32(y);
#endif
}

// <linux/swab.h>

# define swab __swab

// <linux/bits.h>

#define BIT_WORD(nr)		((nr) / BITS_PER_LONG)

// <asm/bitops.h>
//
/**
 * __ffs - find first set bit in word
 * @word: The word to search
 *
 * Undefined if no bit exists, so code should check against 0 first.
 */
static __always_inline unsigned long __ffs(unsigned long word)
{
	asm("rep; bsf %1,%0"
		: "=r" (word)
		: "rm" (word));
	return word;
}


> > But the goal of the function is not to accept the memory, but mark it
> > as unaccepted in the bitmap.
> 
> Really?
> 
> +	 * Accept small regions that might not be able to be represented
> +	 * in the bitmap:
> +	 */
> +	if (end - start < 2 * PMD_SIZE) {
> +		__accept_memory(start, end);
> 
> That looks like it is accepting to me.

Yes, really.

As 1 bit represents 2M, not all chunks can be represented in the bitmap
and they have to be accepted. But the *goal* is to record unaccepted
memory into bitmap. Some accepting is a side effect.

The early_accept_memory() name is just wrong.

> > Conceptionally, it is just memory that requires additional action before
> > it can be accessed. Yes, at the moment TDX and SEV are the only users.
> > It is implementation detail that TDX and SEV use memory encryption.
> 
> So there *might* be some potential future use. Nothing concrete at the
> moment.
> 
> > Because I don't think it is a good fit. Frankly, even <asm/coco.h> fits
> > better, although I'm no a fan either.
> > 
> > Do we have file shortage? I would rather keep it separate.
> 
> So I have not read a single argument for why the unaccepted memory gunk
> should be separate.

> We have perfectly fine mem_encrypt.[ch] files everywhere which already
> contain code which deals with the kernel running as encrypted guest.

And some stuff for encrypted host (SME).

> The unaccepted memory stuff is part of that - not something separate. If
> it gets to get used for something different, sure, then it can be carved
> out because it might need to be built separately, without the rest of
> the encryption code. But as it is now, it doesn't have to. So please put
> it in those files.

Okay, I will do as you want, but I really hate it.

With one hand you try to unwind header mess in decompresser code and with
another propose to create a kitchen-sink header because topics somewhat
related. Looks contradictory.

-- 
 Kirill A. Shutemov