linux-kernel - Re: [PATCH Part1 RFC v4 15/36] x86/mm: Add support to validate memory when changing C-bit

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, 17 Aug 2021 13:07:40 -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,
        linux-efi@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        linux-coco@...ts.linux.dev, linux-mm@...ck.org,
        linux-crypto@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        Tom Lendacky <thomas.lendacky@....com>,
        "H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sergio Lopez <slp@...hat.com>, Peter Gonda <pgonda@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Dov Murik <dovmurik@...ux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@....com>,
        Michael Roth <michael.roth@....com>,
        Vlastimil Babka <vbabka@...e.cz>, tony.luck@...el.com,
        npmccallum@...hat.com, brijesh.ksingh@...il.com
Subject: Re: [PATCH Part1 RFC v4 15/36] x86/mm: Add support to validate memory
 when changing C-bit



On 8/17/21 12:27 PM, Borislav Petkov wrote:
> 
> The majority of kernel code puts __packed after the struct definition,
> let's put it there too pls, out of the way.
> 
> ...

Noted.

>> +		if (WARN(ret || ghcb->save.sw_exit_info_2,
>> +			 "SEV-SNP: page state change failed ret=%d exit_info_2=%llx\n",
>> +			 ret, ghcb->save.sw_exit_info_2))
>> +			return 1;
> 
> Yikes, you return here and below with interrupts disabled.
> 
> All your returns need to be "goto out;" instead where you do
> 
> out:
>          __sev_put_ghcb(&state);
>          local_irq_restore(flags);
> 
> Yap, you very likely need to put the GHCB too.
> 

Sure, let me revisit this code to fix those path.

>> +		/*
>> +		 * Lets do some sanity check that entry processing is not going
>> +		 * backward. This will happen only if hypervisor is tricking us.
>> +		 */
>> +		if (WARN((hdr->end_entry > end_entry) || (cur_entry > hdr->cur_entry),
>> +			"SEV-SNP: page state change processing going backward, end_entry "
>> +			"(expected %d got %d) cur_entry (expected %d got %d)\n",
>> +			end_entry, hdr->end_entry, cur_entry, hdr->cur_entry))
>> +			return 1;
> 
> WARNING: quoted string split across lines
> #293: FILE: arch/x86/kernel/sev.c:750:
> +			"SEV-SNP: page state change processing going backward, end_entry "
> +			"(expected %d got %d) cur_entry (expected %d got %d)\n",
> 
> If you're wondering what to do, yes, you can really stretch that string
> and shorten it too:

Okay.

> 
>                  if (WARN((hdr->end_entry > end_entry) || (cur_entry > hdr->cur_entry),
> "SEV-SNP: PSC processing going backwards, end_entry %d (got %d) cur_entry: %d (got %d)\n",
>                           end_entry, hdr->end_entry, cur_entry, hdr->cur_entry))
>                          return 1;
> 
> so that it fits on a single line and grepping can find it.
> 
Noted.

>> +		/* Lets verify that reserved bit is not set in the header*/
>> +		if (WARN(hdr->reserved, "Reserved bit is set in the PSC header\n"))
> 
> psc_entry has a ->reserved field too and since we're iterating over the
> entries...
> 
Sure I can add that check.


>> +
>> +	desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT);
> 
> kzalloc() so that you don't have to memset() later in
> __set_page_state().

Depending on the size, the __set_page_state() can be call multiple times 
so it should clear the desc memory before filling it.

> 
>> +	if (!desc)
>> +		panic("failed to allocate memory");
> 
> Make that error message more distinctive so that *if* it happens, one
> can pinpoint the place in the code where the panic comes from.
> 

Now I am running checkpatch and notice that it complain about the 
message too. I can add a BUG() or WARN() to get the stack trace before 
the crashing.

>> +	while (vaddr < vaddr_end) {
>> +		/*
>> +		 * Calculate the last vaddr that can be fit in one
>> +		 * struct snp_psc_desc.
>> +		 */
>> +		next_vaddr = min_t(unsigned long, vaddr_end,
>> +				(VMGEXIT_PSC_MAX_ENTRY * PAGE_SIZE) + vaddr);
>> +
>> +		__set_page_state(desc, vaddr, next_vaddr, op);
>> +
>> +		vaddr = next_vaddr;
>> +	}
>> +
>> +	kfree(desc);
>> +}
>> +
>