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

[Tom Lendacky]

On 11/10/21 12:43 PM, Borislav Petkov wrote:
> On Wed, Nov 10, 2021 at 08:21:21AM -0600, Brijesh Singh wrote:
> > I am assuming you mean add some compile time check to ensure that desc will
> > fit in the shared buffer ?
>
> No:
>
> struct ghcb {
>
> 	...
>
>          u8 shared_buffer[2032];
>
> so that memcpy needs to do:
>
> 	memcpy(ghcb->shared_buffer, desc, min_t(int, 2032, sizeof(*desc)));
>
> with that 2032 behind a proper define, ofc.

2032 => sizeof(ghcb->shared_buffer) ?

The idea is that a full snp_psc_desc structure is meant to fit completely 
in the shared_buffer area. So if there are no compile time checks, then 
the code on the HV side will need to ensure that the input doesn't cause 
the HV to access the structure outside of the shared_buffer area - which, 
IIRC, it does (think protect against a malicious guest), so the min_t() on 
the memcpy should be safe on the guest side.

But given the snp_psc_desc is sized/meant to fit completely in the 
shared_buffer, a compile time check would be a good idea, too, right?

Thanks,
Tom

> > 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.
>
> Yah, sounds like the comment is not needed now. You could put this in
> the commit message, though.
>
> Thx.