Re: [PATCH v6 19/42] x86/mm: Add support to validate memory when changing C-bit

[Brijesh Singh <brijesh.singh@amd.com>]

On 11/9/21 1:34 PM, Borislav Petkov wrote:
> On Fri, Oct 08, 2021 at 01:04:30PM -0500, Brijesh Singh wrote:
>> +static int vmgexit_psc(struct snp_psc_desc *desc)
>> +{
>> +	int cur_entry, end_entry, ret;
>> +	struct snp_psc_desc *data;
>> +	struct ghcb_state state;
>> +	struct ghcb *ghcb;
>> +	struct psc_hdr *hdr;
>> +	unsigned long flags;
> 
>          int cur_entry, end_entry, ret;
>          struct snp_psc_desc *data;
>          struct ghcb_state state;
>          struct psc_hdr *hdr;
>          unsigned long flags;
>          struct ghcb *ghcb;
> 
> that's properly sorted.
> 
Noted.

>> +
>> +	local_irq_save(flags);
> 
> What is that protecting against? Comment about it?
> 
> Aha, __sev_get_ghcb() needs to run with IRQs disabled because it is
> using the per-CPU GHCB.
> 

I will add a comment to clarify it.

>> +
>> +	ghcb = __sev_get_ghcb(&state);
>> +	if (unlikely(!ghcb))
>> +		panic("SEV-SNP: Failed to get GHCB\n");
>> +
>> +	/* Copy the input desc into GHCB shared buffer */
>> +	data = (struct snp_psc_desc *)ghcb->shared_buffer;
>> +	memcpy(ghcb->shared_buffer, desc, sizeof(*desc));
> 
> That shared buffer has a size - check it vs the size of the desc thing.
> 

I am assuming you mean add some compile time check to ensure that desc 
will fit in the shared buffer ?

...

>> +		if (WARN(ret || ghcb->save.sw_exit_info_2,
>> +			 "SEV-SNP: PSC failed ret=%d exit_info_2=%llx\n",
>> +			 ret, ghcb->save.sw_exit_info_2)) {
>> +			ret = 1;
> 
> That ret = 1 goes unused with that "return 0" at the end. It should be
> "return ret" at the end.. Ditto for the others. Audit all your exit
> paths in this function.

Noted.


>> +
>> +		/* Verify that reserved bit is not set */
>> +		if (WARN(hdr->reserved, "Reserved bit is set in the PSC header\n")) {
> 
> Shouldn't that thing happen first after the HV call?
> 

I am okay to move this check before the going backward check.

>> +
>> +		/*
>> +		 * The GHCB specification provides the flexibility to
>> +		 * use either 4K or 2MB page size in the RMP table.
>> +		 * The current SNP support does not keep track of the
>> +		 * page size used in the RMP table. To avoid the
>> +		 * overlap request,
> 
> "avoid overlap request"?
> 
> No clue what that means. In general, that comment is talking about
> something in the future and is more confusing than explaining stuff.
> 

I can drop the overlap comment to avoid the confusion, as you pointed it 
more of the future thing. Basically overlap is the below condition

set_memory_private(gfn=0, page_size=2m)
set_memory_private(gfn=10, page_size=4k)

The RMPUPDATE instruction will detect overlap on the second call and 
return an error to the guest. After we add the support to track the page 
validation state (either in bitmap or page flag), the second call will 
not be issued and thus avoid an overlap errors. For now, we use the 
page_size=4k for all the page state changes from the kernel.



>> use the 4K page size in the RMP
>> +		 * table.
>> +		 */
>> +		e->pagesize = RMP_PG_SIZE_4K;
>> +
>> +		vaddr = vaddr + PAGE_SIZE;
>> +		e++;
>> +		i++;
>> +	}
>> +
>> +	if (vmgexit_psc(data))
>> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PSC);
>> +}
>> +
>> +static void set_page_state(unsigned long vaddr, unsigned int npages, int op)
> 
> Yeah, so this should be named
> 
> set_pages_state - notice the plural "pages"
> 
> because it works on multiple pages, @npages exactly.
> 

Ah, I thought I had it pages but maybe it got renamed sometime back in 
the series.


>> +{
>> +	unsigned long vaddr_end, next_vaddr;
>> +	struct snp_psc_desc *desc;
>> +
>> +	vaddr = vaddr & PAGE_MASK;
>> +	vaddr_end = vaddr + (npages << PAGE_SHIFT);
> 
> Take those two...
> 
>> +
>> +	desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT);
>> +	if (!desc)
>> +		panic("SEV-SNP: failed to allocate memory for PSC descriptor\n");
> 
> 
> ... and put them here.
> 
Noted.


thanks