linux-kernel - Re: [PATCHv4 4/8] x86/boot/compressed: Handle 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]

Date:   Sun, 10 Apr 2022 23:49:52 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Kirill A. Shutemov" <kirill@...temov.name>
Cc:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Borislav Petkov <bp@...en8.de>,
        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>,
        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 4/8] x86/boot/compressed: Handle unaccepted memory

On 4/9/22 13:20, Kirill A. Shutemov wrote:
> On Fri, Apr 08, 2022 at 10:57:17AM -0700, Dave Hansen wrote:
...
>> It's a real shame that we have to duplicate this code.  Is there
>> anything crazy we could do here like
>>
>> #include "../../../lib/find_bit.c"
>>
>> ?
> 
> Well, it would require fracturing source files on the kernel side.
> 
> __bitmap_set() and __bitmap_clear() are now in lib/bitmap.c.
> 
> _find_next_bit() is in lib/find_bit.c.
> 
> Both lib/bitmap.c and lib/find_bit.c have a lot of stuff that are not used
> here. I guess we would need to split them into few pieces to make it in
> sane way. Do you want me to go this path?

I'd be curious if others have any sane ideas for how to do it.

One idea would be to stick most of the implementation in a header that
we can #include.  Then, lib/find_bit.c #includes that header and does
something simple like:

#include "header.h"
int _find_next_bit(...)
{
	return _find_next_bit_from_header();
}
EXPORT_SYMBOL(_find_next_bit);


>>> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
>>> index fa8969fad011..c1d9d71a6615 100644
>>> --- a/arch/x86/boot/compressed/misc.c
>>> +++ b/arch/x86/boot/compressed/misc.c
>>> @@ -18,6 +18,7 @@
>>>  #include "../string.h"
>>>  #include "../voffset.h"
>>>  #include <asm/bootparam_utils.h>
>>> +#include <asm/unaccepted_memory.h>
>>>  
>>>  /*
>>>   * WARNING!!
>>> @@ -43,6 +44,9 @@
>>>  void *memmove(void *dest, const void *src, size_t n);
>>>  #endif
>>>  
>>> +#undef __pa
>>> +#define __pa(x)	((unsigned long)(x))
>>
>> Those #undef's always worry me.  Why is this one needed?
> 
> arch/x86/boot/compressed/misc.c:47:9: warning: '__pa' macro redefined [-Wmacro-redefined]
> #define __pa(x) ((unsigned long)(x))
>         ^
> arch/x86/include/asm/page.h:47:9: note: previous definition is here
> #define __pa(x)         __phys_addr((unsigned long)(x))
> 
> Note that sev.c does the same. At least we are consistent :)

Ugh.  Please do look into fixing this properly.  The SEV folks will
thank you. :)

>>> +void accept_memory(phys_addr_t start, phys_addr_t end)
>>> +{
>>> +	unsigned long *unaccepted_memory;
>>> +	unsigned int rs, re;
>>> +
>>> +	unaccepted_memory = (unsigned long *)boot_params->unaccepted_memory;
>>> +	rs = start / PMD_SIZE;
>>
>> OK, so start is a physical address, PMD_SIZE is 2^21, and 'rs' is an
>> unsigned int.  That means 'rs' can, at most, represent a physical
>> address at 2^(21+32), or 2^53.  That's cutting it a *bit* close, don't
>> you think?
>>
>> Could we please just give 'rs' and 're' real names and make them
>> 'unsigned long's, please?  It will surely save at least one other person
>> from doing math.  The find_next_bit() functions seem to take ulongs anyway.
> 
> Okay. 'range_start' and 'range_end' are good enough names?

Yep, works for me.