linux-kernel - Re: [PATCH Part1 RFC v2 16/20] x86/kernel: Validate rom memory before accessing when SEV-SNP is active

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <588df124-6213-22c4-384f-49fa368bb7ed@amd.com>
Date:   Thu, 27 May 2021 07:12:00 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     brijesh.singh@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        tglx@...utronix.de, jroedel@...e.de, thomas.lendacky@....com,
        pbonzini@...hat.com, mingo@...hat.com, dave.hansen@...el.com,
        rientjes@...gle.com, seanjc@...gle.com, peterz@...radead.org,
        hpa@...or.com, tony.luck@...el.com
Subject: Re: [PATCH Part1 RFC v2 16/20] x86/kernel: Validate rom memory before
 accessing when SEV-SNP is active


On 5/27/21 6:49 AM, Borislav Petkov wrote:
> On Fri, Apr 30, 2021 at 07:16:12AM -0500, Brijesh Singh wrote:
>> +	/*
>> +	 * The ROM memory is not part of the E820 system RAM and is not pre-validated
>> +	 * by the BIOS. The kernel page table maps the ROM region as encrypted memory,
>> +	 * the SEV-SNP requires the encrypted memory must be validated before the
>> +	 * access. Validate the ROM before accessing it.
>> +	 */
>> +	n = ((system_rom_resource.end + 1) - video_rom_resource.start) >> PAGE_SHIFT;
>> +	early_snp_set_memory_private((unsigned long)__va(video_rom_resource.start),
>> +			video_rom_resource.start, n);
> From last review:
>
> I don't like this sprinkling of SNP-special stuff that needs to be done,
> around the tree. Instead, pls define a function called
>
>         snp_prep_memory(unsigned long pa, unsigned int num_pages, enum operation);

In the previous patch we were doing:

if (sev_snp_active()) {

   early_set_memory_private(....)

}

Based on your feedback on other patches, I moved the sev_snp_active()
check inside the function. The callsites can now make unconditional call
to change the page state. After implementing that feedback, I don't see
a strong reason for yet another helper unless I am missing something:

snp_prep_memory(pa, n, SNP_PAGE_PRIVATE) == snp_set_memory_private(pa, n)

snp_prep_memory(pa, n, SNP_PAGE_SHARED) == snp_set_memory_shared(pa, n)

Let me know if you still think that snp_prep_memory() helper is required.

-Brijesh

> or so which does all the manipulation needed and the callsites only
> simply unconditionally call that function so that all detail is
> extracted and optimized away when not config-enabled.
>