linux-kernel - Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when AMD sme enabled

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:   Tue, 3 Jul 2018 18:58:14 +0800
From:   lijiang <lijiang@...hat.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     linux-kernel@...r.kernel.org, mingo@...hat.com, tglx@...utronix.de,
        hpa@...or.com, ebiederm@...ssion.com, joro@...tes.org,
        thomas.lendacky@....com, dyoung@...hat.com,
        kexec@...ts.infradead.org, iommu@...ts.linux-foundation.org,
        bhe@...hat.com
Subject: Re: [PATCH 1/5 V5] Add a function(ioremap_encrypted) for kdump when
 AMD sme enabled

在 2018年07月03日 10:17, lijiang 写道:
> 在 2018年07月02日 18:14, Borislav Petkov 写道:
>> On Mon, Jul 02, 2018 at 03:26:35PM +0800, Lianbo Jiang wrote:
>>> @@ -131,7 +132,8 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
>>>   * caller shouldn't need to know that small detail.
>>>   */
>>>  static void __iomem *__ioremap_caller(resource_size_t phys_addr,
>>> -		unsigned long size, enum page_cache_mode pcm, void *caller)
>>> +		unsigned long size, enum page_cache_mode pcm,
>>> +		void *caller, bool encrypted)
>>
>> So instead of sprinkling that @encrypted argument everywhere and then
>> setting it based on sme_active() ...
>>
>>>  {
>>>  	unsigned long offset, vaddr;
>>>  	resource_size_t last_addr;
>>> @@ -199,7 +201,7 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
>>>  	 * resulting mapping.
>>>  	 */
>>>  	prot = PAGE_KERNEL_IO;
>>> -	if (sev_active() && mem_flags.desc_other)
>>> +	if ((sev_active() && mem_flags.desc_other) || encrypted)
>>
>> ... why can't you simply do your checks:
>>
>> 	sme_active() && is_kdump_kernel()
>>
>> here so that __ioremap_caller() can automatically choose the proper
>> pgprot value when ioremapping the memory in the kdump kernel?
>>
>> And this way the callers don't even have to care whether the memory is
>> encrypted or not?
>>
> Thank you, Boris. I'm very glad to read your comments. That's a good idea, but it has some
> unencrypted memory in kdump mode, for example, the elfcorehdr. In fact, the elfcorehdr and
> notes call the same function(read_from_oldmem->ioremap_cache), in this case, it is very
> difficult to properly remap the memory if the caller don't care whether the memory is encrypted.
> 
Hi, Boris,
About this, maybe I should describe the more details.
For kdump, the elf header finally use the crash kernel reserved memory, it is not an old memory.
When we load the crash kernel image, kexec tools will copy the elf header from encrypted memory
region to unencrypted memory region, we know that a page of memory that is marked encrypted will
be automatically decrypted when read from DRAM if SME is enabled. This operation make us get the
plaintext, that is to say, it becomes unencrypted data, so we need to remap the elfcorehdr in un-
encrypted manners in kdump kernel, but elf notes use an old memory, it is encrypted. That is the
real reason why we need to use the different functions(ioremap_cache/ioremap_encrypted).
If the elf core header is set to be encrypted, we could need to know which part of memory is
allocated for the elfcorehdr in kernel space and change the page attribute, maybe which will have
to modify another common codes and kexec-tools.

If you agree with my explanation, i will add them to patch log and also post it again.
Welcome to review my patches again.

Thanks.
Lianbo

> Regards,
> Lianbo>>>  		prot = pgprot_encrypted(prot);
>>>  
>>>  	switch (pcm) {
>>