[PATCHv2,2/3] x86/tdx: Fix race between set_memory_encrypted() and load_unaligned_zeropad()

  Touching privately mapped GPA that is not properly converted to private
with MapGPA and accepted leads to unrecoverable exit to VMM.

load_unaligned_zeropad() can touch memory that is not owned by the
caller, but just happened to next after the owned memory.
This load_unaligned_zeropad() behaviour makes it important when kernel
asks VMM to convert a GPA from shared to private or back. Kernel must
never have a page mapped into direct mapping (and aliases) as private
when the GPA is already converted to shared or when GPA is not yet
converted to private.

guest.enc_status_change_prepare() called before adjusting direct mapping
and therefore it is responsible for converting the memory to private.

guest.enc_status_change_finish() called after adjusting direct mapping
and it converts the memory to shared.

It is okay to have a shared mapping of memory that is not converted
properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
stepping on it.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Fixes: 7dbde7631629 ("x86/mm/cpa: Add support for TDX shared memory")
Cc: stable@vger.kernel.org
---
 arch/x86/coco/tdx/tdx.c | 56 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 53 insertions(+), 3 deletions(-)
  

On 5/26/23 5:02 AM, Kirill A. Shutemov wrote:
> Touching privately mapped GPA that is not properly converted to private
> with MapGPA and accepted leads to unrecoverable exit to VMM.
> 
> load_unaligned_zeropad() can touch memory that is not owned by the
> caller, but just happened to next after the owned memory.

/s/to/to be ?

> This load_unaligned_zeropad() behaviour makes it important when kernel
> asks VMM to convert a GPA from shared to private or back. Kernel must
> never have a page mapped into direct mapping (and aliases) as private
> when the GPA is already converted to shared or when GPA is not yet
> converted to private.

I am wondering whether this issue exist in the AMD code? 

IMO, you can add some info on the window in set_memory_encrypted()
where this race exists.

> 
> guest.enc_status_change_prepare() called before adjusting direct mapping
> and therefore it is responsible for converting the memory to private.
> 
> guest.enc_status_change_finish() called after adjusting direct mapping
> and it converts the memory to shared.
> 
> It is okay to have a shared mapping of memory that is not converted
> properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
> stepping on it.

> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Fixes: 7dbde7631629 ("x86/mm/cpa: Add support for TDX shared memory")
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/coco/tdx/tdx.c | 56 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 53 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index e146b599260f..59cc13e41aa6 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -840,6 +840,30 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
>  	return true;
>  }
>  
> +static bool tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> +					  bool enc)
> +{
> +	/*
> +	 * Only handle shared->private conversion here.
> +	 * See the comment in tdx_early_init().
> +	 */
> +	if (enc)
> +		return tdx_enc_status_changed(vaddr, numpages, enc);
> +	return true;
> +}
> +
> +static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> +					 bool enc)
> +{
> +	/*
> +	 * Only handle private->shared conversion here.
> +	 * See the comment in tdx_early_init().
> +	 */
> +	if (!enc)
> +		return tdx_enc_status_changed(vaddr, numpages, enc);
> +	return true;
> +}
> +
>  void __init tdx_early_init(void)
>  {
>  	u64 cc_mask;
> @@ -867,9 +891,35 @@ void __init tdx_early_init(void)
>  	 */
>  	physical_mask &= cc_mask - 1;
>  
> -	x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required;
> -	x86_platform.guest.enc_tlb_flush_required   = tdx_tlb_flush_required;

I think you don't need to change the order here.

> -	x86_platform.guest.enc_status_change_finish = tdx_enc_status_changed;
> +	/*
> +	 * Touching privately mapped GPA that is not properly converted to
> +	 * private with MapGPA and accepted leads to unrecoverable exit
> +	 * to VMM.
> +	 *
> +	 * load_unaligned_zeropad() can touch memory that is not owned by
> +	 * the caller, but just happened to next after the owned memory.
> +	 * This load_unaligned_zeropad() behaviour makes it important when
> +	 * kernel asks VMM to convert a GPA from shared to private or back.
> +	 * Kernel must never have a page mapped into direct mapping (and
> +	 * aliases) as private when the GPA is already converted to shared or
> +	 * when GPA is not yet converted to private.
> +	 *
> +	 * guest.enc_status_change_prepare() called before adjusting direct
> +	 * mapping and therefore it is responsible for converting the memory
> +	 * to private.
> +	 *
> +	 * guest.enc_status_change_finish() called after adjusting direct
> +	 * mapping and it converts the memory to shared.
> +	 *
> +	 * It is okay to have a shared mapping of memory that is not converted
> +	 * properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
> +	 * stepping on it.
> +	 */
> +	x86_platform.guest.enc_status_change_prepare = tdx_enc_status_change_prepare;
> +	x86_platform.guest.enc_status_change_finish  = tdx_enc_status_change_finish;
> +
> +	x86_platform.guest.enc_cache_flush_required  = tdx_cache_flush_required;
> +	x86_platform.guest.enc_tlb_flush_required    = tdx_tlb_flush_required;
>  
>  	pr_info("Guest detected\n");
>  }
  

On Fri, May 26, 2023 at 03:10:56PM -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 5/26/23 5:02 AM, Kirill A. Shutemov wrote:
> > Touching privately mapped GPA that is not properly converted to private
> > with MapGPA and accepted leads to unrecoverable exit to VMM.
> > 
> > load_unaligned_zeropad() can touch memory that is not owned by the
> > caller, but just happened to next after the owned memory.
> 
> /s/to/to be ?

Yep, my bad.

> > This load_unaligned_zeropad() behaviour makes it important when kernel
> > asks VMM to convert a GPA from shared to private or back. Kernel must
> > never have a page mapped into direct mapping (and aliases) as private
> > when the GPA is already converted to shared or when GPA is not yet
> > converted to private.
> 
> I am wondering whether this issue exist in the AMD code? 
> 
> IMO, you can add some info on the window in set_memory_encrypted()
> where this race exists.

I don't think AMD affected by load_unaligned_zeropad() the same way as
Intel does. But I'm not sure.

Tom, do you have any comments?

> 
> > 
> > guest.enc_status_change_prepare() called before adjusting direct mapping
> > and therefore it is responsible for converting the memory to private.
> > 
> > guest.enc_status_change_finish() called after adjusting direct mapping
> > and it converts the memory to shared.
> > 
> > It is okay to have a shared mapping of memory that is not converted
> > properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
> > stepping on it.
> 
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Fixes: 7dbde7631629 ("x86/mm/cpa: Add support for TDX shared memory")
> > Cc: stable@vger.kernel.org
> > ---
> >  arch/x86/coco/tdx/tdx.c | 56 ++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 53 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> > index e146b599260f..59cc13e41aa6 100644
> > --- a/arch/x86/coco/tdx/tdx.c
> > +++ b/arch/x86/coco/tdx/tdx.c
> > @@ -840,6 +840,30 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> >  	return true;
> >  }
> >  
> > +static bool tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> > +					  bool enc)
> > +{
> > +	/*
> > +	 * Only handle shared->private conversion here.
> > +	 * See the comment in tdx_early_init().
> > +	 */
> > +	if (enc)
> > +		return tdx_enc_status_changed(vaddr, numpages, enc);
> > +	return true;
> > +}
> > +
> > +static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> > +					 bool enc)
> > +{
> > +	/*
> > +	 * Only handle private->shared conversion here.
> > +	 * See the comment in tdx_early_init().
> > +	 */
> > +	if (!enc)
> > +		return tdx_enc_status_changed(vaddr, numpages, enc);
> > +	return true;
> > +}
> > +
> >  void __init tdx_early_init(void)
> >  {
> >  	u64 cc_mask;
> > @@ -867,9 +891,35 @@ void __init tdx_early_init(void)
> >  	 */
> >  	physical_mask &= cc_mask - 1;
> >  
> > -	x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required;
> > -	x86_platform.guest.enc_tlb_flush_required   = tdx_tlb_flush_required;
> 
> I think you don't need to change the order here.

I wanted to emphasise that the comment is for _prepare/_finish callbacks
and I hoped re-order would help with this.

> > -	x86_platform.guest.enc_status_change_finish = tdx_enc_status_changed;
> > +	/*
> > +	 * Touching privately mapped GPA that is not properly converted to
> > +	 * private with MapGPA and accepted leads to unrecoverable exit
> > +	 * to VMM.
> > +	 *
> > +	 * load_unaligned_zeropad() can touch memory that is not owned by
> > +	 * the caller, but just happened to next after the owned memory.
> > +	 * This load_unaligned_zeropad() behaviour makes it important when
> > +	 * kernel asks VMM to convert a GPA from shared to private or back.
> > +	 * Kernel must never have a page mapped into direct mapping (and
> > +	 * aliases) as private when the GPA is already converted to shared or
> > +	 * when GPA is not yet converted to private.
> > +	 *
> > +	 * guest.enc_status_change_prepare() called before adjusting direct
> > +	 * mapping and therefore it is responsible for converting the memory
> > +	 * to private.
> > +	 *
> > +	 * guest.enc_status_change_finish() called after adjusting direct
> > +	 * mapping and it converts the memory to shared.
> > +	 *
> > +	 * It is okay to have a shared mapping of memory that is not converted
> > +	 * properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
> > +	 * stepping on it.
> > +	 */
> > +	x86_platform.guest.enc_status_change_prepare = tdx_enc_status_change_prepare;
> > +	x86_platform.guest.enc_status_change_finish  = tdx_enc_status_change_finish;
> > +
> > +	x86_platform.guest.enc_cache_flush_required  = tdx_cache_flush_required;
> > +	x86_platform.guest.enc_tlb_flush_required    = tdx_tlb_flush_required;
> >  
> >  	pr_info("Guest detected\n");
> >  }
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
  

On 5/29/23 19:57, Kirill A. Shutemov wrote:
> On Fri, May 26, 2023 at 03:10:56PM -0700, Sathyanarayanan Kuppuswamy wrote:
>>
>>
>> On 5/26/23 5:02 AM, Kirill A. Shutemov wrote:
>>> Touching privately mapped GPA that is not properly converted to private
>>> with MapGPA and accepted leads to unrecoverable exit to VMM.
>>>
>>> load_unaligned_zeropad() can touch memory that is not owned by the
>>> caller, but just happened to next after the owned memory.
>>
>> /s/to/to be ?
> 
> Yep, my bad.
> 
>>> This load_unaligned_zeropad() behaviour makes it important when kernel
>>> asks VMM to convert a GPA from shared to private or back. Kernel must
>>> never have a page mapped into direct mapping (and aliases) as private
>>> when the GPA is already converted to shared or when GPA is not yet
>>> converted to private.
>>
>> I am wondering whether this issue exist in the AMD code?
>>
>> IMO, you can add some info on the window in set_memory_encrypted()
>> where this race exists.
> 
> I don't think AMD affected by load_unaligned_zeropad() the same way as
> Intel does. But I'm not sure.
> 
> Tom, do you have any comments?

Right, shouldn't be an issue for SNP.

Thanks,
Tom

> 
>>
>>>
>>> guest.enc_status_change_prepare() called before adjusting direct mapping
>>> and therefore it is responsible for converting the memory to private.
>>>
>>> guest.enc_status_change_finish() called after adjusting direct mapping
>>> and it converts the memory to shared.
>>>
>>> It is okay to have a shared mapping of memory that is not converted
>>> properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
>>> stepping on it.
>>
>>>
>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Fixes: 7dbde7631629 ("x86/mm/cpa: Add support for TDX shared memory")
>>> Cc: stable@vger.kernel.org
>>> ---
>>>   arch/x86/coco/tdx/tdx.c | 56 ++++++++++++++++++++++++++++++++++++++---
>>>   1 file changed, 53 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>>> index e146b599260f..59cc13e41aa6 100644
>>> --- a/arch/x86/coco/tdx/tdx.c
>>> +++ b/arch/x86/coco/tdx/tdx.c
>>> @@ -840,6 +840,30 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
>>>   	return true;
>>>   }
>>>   
>>> +static bool tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
>>> +					  bool enc)
>>> +{
>>> +	/*
>>> +	 * Only handle shared->private conversion here.
>>> +	 * See the comment in tdx_early_init().
>>> +	 */
>>> +	if (enc)
>>> +		return tdx_enc_status_changed(vaddr, numpages, enc);
>>> +	return true;
>>> +}
>>> +
>>> +static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
>>> +					 bool enc)
>>> +{
>>> +	/*
>>> +	 * Only handle private->shared conversion here.
>>> +	 * See the comment in tdx_early_init().
>>> +	 */
>>> +	if (!enc)
>>> +		return tdx_enc_status_changed(vaddr, numpages, enc);
>>> +	return true;
>>> +}
>>> +
>>>   void __init tdx_early_init(void)
>>>   {
>>>   	u64 cc_mask;
>>> @@ -867,9 +891,35 @@ void __init tdx_early_init(void)
>>>   	 */
>>>   	physical_mask &= cc_mask - 1;
>>>   
>>> -	x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required;
>>> -	x86_platform.guest.enc_tlb_flush_required   = tdx_tlb_flush_required;
>>
>> I think you don't need to change the order here.
> 
> I wanted to emphasise that the comment is for _prepare/_finish callbacks
> and I hoped re-order would help with this.
> 
>>> -	x86_platform.guest.enc_status_change_finish = tdx_enc_status_changed;
>>> +	/*
>>> +	 * Touching privately mapped GPA that is not properly converted to
>>> +	 * private with MapGPA and accepted leads to unrecoverable exit
>>> +	 * to VMM.
>>> +	 *
>>> +	 * load_unaligned_zeropad() can touch memory that is not owned by
>>> +	 * the caller, but just happened to next after the owned memory.
>>> +	 * This load_unaligned_zeropad() behaviour makes it important when
>>> +	 * kernel asks VMM to convert a GPA from shared to private or back.
>>> +	 * Kernel must never have a page mapped into direct mapping (and
>>> +	 * aliases) as private when the GPA is already converted to shared or
>>> +	 * when GPA is not yet converted to private.
>>> +	 *
>>> +	 * guest.enc_status_change_prepare() called before adjusting direct
>>> +	 * mapping and therefore it is responsible for converting the memory
>>> +	 * to private.
>>> +	 *
>>> +	 * guest.enc_status_change_finish() called after adjusting direct
>>> +	 * mapping and it converts the memory to shared.
>>> +	 *
>>> +	 * It is okay to have a shared mapping of memory that is not converted
>>> +	 * properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
>>> +	 * stepping on it.
>>> +	 */
>>> +	x86_platform.guest.enc_status_change_prepare = tdx_enc_status_change_prepare;
>>> +	x86_platform.guest.enc_status_change_finish  = tdx_enc_status_change_finish;
>>> +
>>> +	x86_platform.guest.enc_cache_flush_required  = tdx_cache_flush_required;
>>> +	x86_platform.guest.enc_tlb_flush_required    = tdx_tlb_flush_required;
>>>   
>>>   	pr_info("Guest detected\n");
>>>   }
>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
>
  

Hi,

On 5/30/23 5:57 AM, Tom Lendacky wrote:
> On 5/29/23 19:57, Kirill A. Shutemov wrote:
>> On Fri, May 26, 2023 at 03:10:56PM -0700, Sathyanarayanan Kuppuswamy wrote:
>>>
>>>
>>> On 5/26/23 5:02 AM, Kirill A. Shutemov wrote:
>>>> Touching privately mapped GPA that is not properly converted to private
>>>> with MapGPA and accepted leads to unrecoverable exit to VMM.
>>>>
>>>> load_unaligned_zeropad() can touch memory that is not owned by the
>>>> caller, but just happened to next after the owned memory.
>>>
>>> /s/to/to be ?
>>
>> Yep, my bad.
>>
>>>> This load_unaligned_zeropad() behaviour makes it important when kernel
>>>> asks VMM to convert a GPA from shared to private or back. Kernel must
>>>> never have a page mapped into direct mapping (and aliases) as private
>>>> when the GPA is already converted to shared or when GPA is not yet
>>>> converted to private.
>>>
>>> I am wondering whether this issue exist in the AMD code?
>>>
>>> IMO, you can add some info on the window in set_memory_encrypted()
>>> where this race exists.
>>
>> I don't think AMD affected by load_unaligned_zeropad() the same way as
>> Intel does. But I'm not sure.
>>
>> Tom, do you have any comments?
> 
> Right, shouldn't be an issue for SNP.

Thanks for confirming.

> 
> Thanks,
> Tom
> 
>>
>>>
>>>>
>>>> guest.enc_status_change_prepare() called before adjusting direct mapping
>>>> and therefore it is responsible for converting the memory to private.
>>>>
>>>> guest.enc_status_change_finish() called after adjusting direct mapping
>>>> and it converts the memory to shared.
>>>>
>>>> It is okay to have a shared mapping of memory that is not converted
>>>> properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
>>>> stepping on it.
>>>
>>>>

Rest looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>> Fixes: 7dbde7631629 ("x86/mm/cpa: Add support for TDX shared memory")
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>   arch/x86/coco/tdx/tdx.c | 56 ++++++++++++++++++++++++++++++++++++++---
>>>>   1 file changed, 53 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>>>> index e146b599260f..59cc13e41aa6 100644
>>>> --- a/arch/x86/coco/tdx/tdx.c
>>>> +++ b/arch/x86/coco/tdx/tdx.c
>>>> @@ -840,6 +840,30 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
>>>>       return true;
>>>>   }
>>>>   +static bool tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
>>>> +                      bool enc)
>>>> +{
>>>> +    /*
>>>> +     * Only handle shared->private conversion here.
>>>> +     * See the comment in tdx_early_init().
>>>> +     */
>>>> +    if (enc)
>>>> +        return tdx_enc_status_changed(vaddr, numpages, enc);
>>>> +    return true;
>>>> +}
>>>> +
>>>> +static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
>>>> +                     bool enc)
>>>> +{
>>>> +    /*
>>>> +     * Only handle private->shared conversion here.
>>>> +     * See the comment in tdx_early_init().
>>>> +     */
>>>> +    if (!enc)
>>>> +        return tdx_enc_status_changed(vaddr, numpages, enc);
>>>> +    return true;
>>>> +}
>>>> +
>>>>   void __init tdx_early_init(void)
>>>>   {
>>>>       u64 cc_mask;
>>>> @@ -867,9 +891,35 @@ void __init tdx_early_init(void)
>>>>        */
>>>>       physical_mask &= cc_mask - 1;
>>>>   -    x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required;
>>>> -    x86_platform.guest.enc_tlb_flush_required   = tdx_tlb_flush_required;
>>>
>>> I think you don't need to change the order here.
>>
>> I wanted to emphasise that the comment is for _prepare/_finish callbacks
>> and I hoped re-order would help with this.
>>
>>>> -    x86_platform.guest.enc_status_change_finish = tdx_enc_status_changed;
>>>> +    /*
>>>> +     * Touching privately mapped GPA that is not properly converted to
>>>> +     * private with MapGPA and accepted leads to unrecoverable exit
>>>> +     * to VMM.
>>>> +     *
>>>> +     * load_unaligned_zeropad() can touch memory that is not owned by
>>>> +     * the caller, but just happened to next after the owned memory.
>>>> +     * This load_unaligned_zeropad() behaviour makes it important when
>>>> +     * kernel asks VMM to convert a GPA from shared to private or back.
>>>> +     * Kernel must never have a page mapped into direct mapping (and
>>>> +     * aliases) as private when the GPA is already converted to shared or
>>>> +     * when GPA is not yet converted to private.
>>>> +     *
>>>> +     * guest.enc_status_change_prepare() called before adjusting direct
>>>> +     * mapping and therefore it is responsible for converting the memory
>>>> +     * to private.
>>>> +     *
>>>> +     * guest.enc_status_change_finish() called after adjusting direct
>>>> +     * mapping and it converts the memory to shared.
>>>> +     *
>>>> +     * It is okay to have a shared mapping of memory that is not converted
>>>> +     * properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
>>>> +     * stepping on it.
>>>> +     */
>>>> +    x86_platform.guest.enc_status_change_prepare = tdx_enc_status_change_prepare;
>>>> +    x86_platform.guest.enc_status_change_finish  = tdx_enc_status_change_finish;
>>>> +
>>>> +    x86_platform.guest.enc_cache_flush_required  = tdx_cache_flush_required;
>>>> +    x86_platform.guest.enc_tlb_flush_required    = tdx_tlb_flush_required;
>>>>         pr_info("Guest detected\n");
>>>>   }
>>>
>>> -- 
>>> Sathyanarayanan Kuppuswamy
>>> Linux Kernel Developer
>>
  

From: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>
Sent: Tuesday, May 30, 2023 6:22 AM
> 
> Hi,
> 
> On 5/30/23 5:57 AM, Tom Lendacky wrote:
> > On 5/29/23 19:57, Kirill A. Shutemov wrote:
> >> On Fri, May 26, 2023 at 03:10:56PM -0700, Sathyanarayanan Kuppuswamy wrote:
> >>>
> >>>
> >>> On 5/26/23 5:02 AM, Kirill A. Shutemov wrote:
> >>>> Touching privately mapped GPA that is not properly converted to private
> >>>> with MapGPA and accepted leads to unrecoverable exit to VMM.
> >>>>
> >>>> load_unaligned_zeropad() can touch memory that is not owned by the
> >>>> caller, but just happened to next after the owned memory.
> >>>
> >>> /s/to/to be ?
> >>
> >> Yep, my bad.
> >>
> >>>> This load_unaligned_zeropad() behaviour makes it important when kernel
> >>>> asks VMM to convert a GPA from shared to private or back. Kernel must
> >>>> never have a page mapped into direct mapping (and aliases) as private
> >>>> when the GPA is already converted to shared or when GPA is not yet
> >>>> converted to private.
> >>>
> >>> I am wondering whether this issue exist in the AMD code?
> >>>
> >>> IMO, you can add some info on the window in set_memory_encrypted()
> >>> where this race exists.
> >>
> >> I don't think AMD affected by load_unaligned_zeropad() the same way as
> >> Intel does. But I'm not sure.
> >>
> >> Tom, do you have any comments?
> >
> > Right, shouldn't be an issue for SNP.
> 
> Thanks for confirming.
> 

Tom -- For my education, could you elaborate on why this problem can't
occur in an SEV-SNP guest?  There's still a window where the direct map
PTE and the RMP as maintained by the hypervisor are out-of-sync.  If
load_unaligned_zeropad() does a read using the direct map PTE during
this out-of-sync window, isn't that going to trap to the hypervisor?  How
is the scenario is handled from there to provide the zeros to
load_unaligned_zeropad()?  I need to make sure Hyper-V is doing whatever
is needed. :-)  

Thanks,

Michael
  

On 5/31/23 15:00, Michael Kelley (LINUX) wrote:
> From: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>
> Sent: Tuesday, May 30, 2023 6:22 AM
>>
>> Hi,
>>
>> On 5/30/23 5:57 AM, Tom Lendacky wrote:
>>> On 5/29/23 19:57, Kirill A. Shutemov wrote:
>>>> On Fri, May 26, 2023 at 03:10:56PM -0700, Sathyanarayanan Kuppuswamy wrote:
>>>>>
>>>>>
>>>>> On 5/26/23 5:02 AM, Kirill A. Shutemov wrote:
>>>>>> Touching privately mapped GPA that is not properly converted to private
>>>>>> with MapGPA and accepted leads to unrecoverable exit to VMM.
>>>>>>
>>>>>> load_unaligned_zeropad() can touch memory that is not owned by the
>>>>>> caller, but just happened to next after the owned memory.
>>>>>
>>>>> /s/to/to be ?
>>>>
>>>> Yep, my bad.
>>>>
>>>>>> This load_unaligned_zeropad() behaviour makes it important when kernel
>>>>>> asks VMM to convert a GPA from shared to private or back. Kernel must
>>>>>> never have a page mapped into direct mapping (and aliases) as private
>>>>>> when the GPA is already converted to shared or when GPA is not yet
>>>>>> converted to private.
>>>>>
>>>>> I am wondering whether this issue exist in the AMD code?
>>>>>
>>>>> IMO, you can add some info on the window in set_memory_encrypted()
>>>>> where this race exists.
>>>>
>>>> I don't think AMD affected by load_unaligned_zeropad() the same way as
>>>> Intel does. But I'm not sure.
>>>>
>>>> Tom, do you have any comments?
>>>
>>> Right, shouldn't be an issue for SNP.
>>
>> Thanks for confirming.
>>
> 
> Tom -- For my education, could you elaborate on why this problem can't
> occur in an SEV-SNP guest?  There's still a window where the direct map
> PTE and the RMP as maintained by the hypervisor are out-of-sync.  If
> load_unaligned_zeropad() does a read using the direct map PTE during
> this out-of-sync window, isn't that going to trap to the hypervisor?  How
> is the scenario is handled from there to provide the zeros to
> load_unaligned_zeropad()?  I need to make sure Hyper-V is doing whatever
> is needed. :-)

Ah, I think I misunderstood this when it was being talked about. The issue 
SNP would have would be between setting the c-bit but before the PVALIDATE 
is issued. Prior to the RMP being updated, referencing the page will 
generate an #NPF and automatically change the RMP over to private (in 
KVM). However, after the guest is resumed, the page will not have been 
validated resulting in a #VC with error code 0x404 being generated, 
causing the guest to terminate itself.

I suppose, when a 0x404 error code is encountered by the #VC handler, it 
could call search_exception_tables() and call ex_handler_zeropad() for the 
EX_TYPE_ZEROPAD type (ex_handler_zeropad is currently static, though).

Thanks,
Tom

> 
> Thanks,
> 
> Michael
> 
>
  

From: Tom Lendacky <thomas.lendacky@amd.com> Sent: Thursday, June 1, 2023 11:19 AM
> 
> On 5/31/23 15:00, Michael Kelley (LINUX) wrote:
> > From: Sathyanarayanan Kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Sent: Tuesday, May 30, 2023 6:22 AM
> >>
> >> Hi,
> >>
> >> On 5/30/23 5:57 AM, Tom Lendacky wrote:
> >>> On 5/29/23 19:57, Kirill A. Shutemov wrote:
> >>>> On Fri, May 26, 2023 at 03:10:56PM -0700, Sathyanarayanan Kuppuswamy
> wrote:
> >>>>>
> >>>>>
> >>>>> On 5/26/23 5:02 AM, Kirill A. Shutemov wrote:
> >>>>>> Touching privately mapped GPA that is not properly converted to private
> >>>>>> with MapGPA and accepted leads to unrecoverable exit to VMM.
> >>>>>>
> >>>>>> load_unaligned_zeropad() can touch memory that is not owned by the
> >>>>>> caller, but just happened to next after the owned memory.
> >>>>>
> >>>>> /s/to/to be ?
> >>>>
> >>>> Yep, my bad.
> >>>>
> >>>>>> This load_unaligned_zeropad() behaviour makes it important when kernel
> >>>>>> asks VMM to convert a GPA from shared to private or back. Kernel must
> >>>>>> never have a page mapped into direct mapping (and aliases) as private
> >>>>>> when the GPA is already converted to shared or when GPA is not yet
> >>>>>> converted to private.
> >>>>>
> >>>>> I am wondering whether this issue exist in the AMD code?
> >>>>>
> >>>>> IMO, you can add some info on the window in set_memory_encrypted()
> >>>>> where this race exists.
> >>>>
> >>>> I don't think AMD affected by load_unaligned_zeropad() the same way as
> >>>> Intel does. But I'm not sure.
> >>>>
> >>>> Tom, do you have any comments?
> >>>
> >>> Right, shouldn't be an issue for SNP.
> >>
> >> Thanks for confirming.
> >>
> >
> > Tom -- For my education, could you elaborate on why this problem can't
> > occur in an SEV-SNP guest?  There's still a window where the direct map
> > PTE and the RMP as maintained by the hypervisor are out-of-sync.  If
> > load_unaligned_zeropad() does a read using the direct map PTE during
> > this out-of-sync window, isn't that going to trap to the hypervisor?  How
> > is the scenario is handled from there to provide the zeros to
> > load_unaligned_zeropad()?  I need to make sure Hyper-V is doing whatever
> > is needed. :-)
> 
> Ah, I think I misunderstood this when it was being talked about. The issue
> SNP would have would be between setting the c-bit but before the PVALIDATE
> is issued. Prior to the RMP being updated, referencing the page will
> generate an #NPF and automatically change the RMP over to private (in
> KVM). However, after the guest is resumed, the page will not have been
> validated resulting in a #VC with error code 0x404 being generated,
> causing the guest to terminate itself.
> 
> I suppose, when a 0x404 error code is encountered by the #VC handler, it
> could call search_exception_tables() and call ex_handler_zeropad() for the
> EX_TYPE_ZEROPAD type (ex_handler_zeropad is currently static, though).
> 

Tom -- Does the above sequence *depend* on the hypervisor doing anything
to make it work?  I'm not clear on why KVM would automatically change the
page over to private.  If there's a dependency on the hypervisor doing
something, then it seems like we'll need to standardize that "something"
across hypervisors, lest we end up with per-hypervisor code in Linux to handle
this scenario.  And running SEV-SNP with multiple VMPLs probably makes it
even more complicated.

Kirill -- Same question about TDX.  Does making load_unaligned_zeropad()
work in a TDX VM depend on the hypervisor doing anything?  Or is the
behavior seen by the guest dependent only on architected behavior of
the TDX processor?

Looking at this problem from a slightly higher level, and thinking out loud
a bit, load_unaligned_zeropad() functionality is provided only for certain
architectures:  x86/64, arm, arm64, and PowerPC 64 (little endian).  There are
fallbacks for architectures that don't support it.  With two minor tweaks to
Kconfig files, I've built x86 with load_unaligned_zeropad() disabled. Maybe
with today's processors the performance benefits are past their prime,
and running with it disabled in CoCo VMs is the better solution.  Does
anyone have a sense of whether the perf impact would be measureable?

If doing the load_unaligned_zeropad() enable/disable at build time is too
limiting, maybe it could be runtime based on whether page private/shared
state is being enforced.  I haven't looked at the details.

Thoughts?

Michael
  

On 6/2/23 11:11, Michael Kelley (LINUX) wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com> Sent: Thursday, June 1, 2023 11:19 AM
>>
>> On 5/31/23 15:00, Michael Kelley (LINUX) wrote:
>>> From: Sathyanarayanan Kuppuswamy
>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> Sent: Tuesday, May 30, 2023 6:22 AM
>>>>
>>>> Hi,
>>>>
>>>> On 5/30/23 5:57 AM, Tom Lendacky wrote:
>>>>> On 5/29/23 19:57, Kirill A. Shutemov wrote:
>>>>>> On Fri, May 26, 2023 at 03:10:56PM -0700, Sathyanarayanan Kuppuswamy
>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/26/23 5:02 AM, Kirill A. Shutemov wrote:
>>>>>>>> Touching privately mapped GPA that is not properly converted to private
>>>>>>>> with MapGPA and accepted leads to unrecoverable exit to VMM.
>>>>>>>>
>>>>>>>> load_unaligned_zeropad() can touch memory that is not owned by the
>>>>>>>> caller, but just happened to next after the owned memory.
>>>>>>>
>>>>>>> /s/to/to be ?
>>>>>>
>>>>>> Yep, my bad.
>>>>>>
>>>>>>>> This load_unaligned_zeropad() behaviour makes it important when kernel
>>>>>>>> asks VMM to convert a GPA from shared to private or back. Kernel must
>>>>>>>> never have a page mapped into direct mapping (and aliases) as private
>>>>>>>> when the GPA is already converted to shared or when GPA is not yet
>>>>>>>> converted to private.
>>>>>>>
>>>>>>> I am wondering whether this issue exist in the AMD code?
>>>>>>>
>>>>>>> IMO, you can add some info on the window in set_memory_encrypted()
>>>>>>> where this race exists.
>>>>>>
>>>>>> I don't think AMD affected by load_unaligned_zeropad() the same way as
>>>>>> Intel does. But I'm not sure.
>>>>>>
>>>>>> Tom, do you have any comments?
>>>>>
>>>>> Right, shouldn't be an issue for SNP.
>>>>
>>>> Thanks for confirming.
>>>>
>>>
>>> Tom -- For my education, could you elaborate on why this problem can't
>>> occur in an SEV-SNP guest?  There's still a window where the direct map
>>> PTE and the RMP as maintained by the hypervisor are out-of-sync.  If
>>> load_unaligned_zeropad() does a read using the direct map PTE during
>>> this out-of-sync window, isn't that going to trap to the hypervisor?  How
>>> is the scenario is handled from there to provide the zeros to
>>> load_unaligned_zeropad()?  I need to make sure Hyper-V is doing whatever
>>> is needed. :-)
>>
>> Ah, I think I misunderstood this when it was being talked about. The issue
>> SNP would have would be between setting the c-bit but before the PVALIDATE
>> is issued. Prior to the RMP being updated, referencing the page will
>> generate an #NPF and automatically change the RMP over to private (in
>> KVM). However, after the guest is resumed, the page will not have been
>> validated resulting in a #VC with error code 0x404 being generated,
>> causing the guest to terminate itself.
>>
>> I suppose, when a 0x404 error code is encountered by the #VC handler, it
>> could call search_exception_tables() and call ex_handler_zeropad() for the
>> EX_TYPE_ZEROPAD type (ex_handler_zeropad is currently static, though).
>>
> 
> Tom -- Does the above sequence *depend* on the hypervisor doing anything
> to make it work?  I'm not clear on why KVM would automatically change the
> page over to private.  If there's a dependency on the hypervisor doing
> something, then it seems like we'll need to standardize that "something"
> across hypervisors, lest we end up with per-hypervisor code in Linux to handle
> this scenario.  And running SEV-SNP with multiple VMPLs probably makes it
> even more complicated.

No, it doesn't depend on the hypervisor doing anything. If the RMP hasn't 
been updated, then a #VMEXIT(NPF) will be triggered (see APM vol 2, Table 
15-39). The hypervisor is free to do what it wants with that, e.g. just 
resume the guest (and immediately take another #VMEXIT(NPF), possibly). 
Since it is a different thread/vCPU trying to access the memory than is 
changing the state of the memory, eventually the guest kernel thread that 
is changing the state of the memory will issue the page state change to 
the hypervisor and the other thread can continue.

Thanks,
Tom

> 
> Kirill -- Same question about TDX.  Does making load_unaligned_zeropad()
> work in a TDX VM depend on the hypervisor doing anything?  Or is the
> behavior seen by the guest dependent only on architected behavior of
> the TDX processor?
> 
> Looking at this problem from a slightly higher level, and thinking out loud
> a bit, load_unaligned_zeropad() functionality is provided only for certain
> architectures:  x86/64, arm, arm64, and PowerPC 64 (little endian).  There are
> fallbacks for architectures that don't support it.  With two minor tweaks to
> Kconfig files, I've built x86 with load_unaligned_zeropad() disabled. Maybe
> with today's processors the performance benefits are past their prime,
> and running with it disabled in CoCo VMs is the better solution.  Does
> anyone have a sense of whether the perf impact would be measureable?
> 
> If doing the load_unaligned_zeropad() enable/disable at build time is too
> limiting, maybe it could be runtime based on whether page private/shared
> state is being enforced.  I haven't looked at the details.
> 
> Thoughts?
> 
> Michael
  

On 6/2/23 09:11, Michael Kelley (LINUX) wrote:
> Tom -- Does the above sequence *depend* on the hypervisor doing anything
> to make it work?  I'm not clear on why KVM would automatically change the
> page over to private.  If there's a dependency on the hypervisor doing
> something, then it seems like we'll need to standardize that "something"
> across hypervisors, lest we end up with per-hypervisor code in Linux to handle
> this scenario.  And running SEV-SNP with multiple VMPLs probably makes it
> even more complicated.
> 
> Kirill -- Same question about TDX.  Does making load_unaligned_zeropad()
> work in a TDX VM depend on the hypervisor doing anything?  Or is the
> behavior seen by the guest dependent only on architected behavior of
> the TDX processor?

No, there's no active help from the hypervisor here.

Also, fwiw, the "architected behavior" here is really just the TDX
module policy and _arguably_ the hardware Secure-EPT controlled by the
TDX module.
  

On Fri, Jun 02, 2023 at 10:42:33AM -0700, Dave Hansen wrote:
> On 6/2/23 09:11, Michael Kelley (LINUX) wrote:
> > Tom -- Does the above sequence *depend* on the hypervisor doing anything
> > to make it work?  I'm not clear on why KVM would automatically change the
> > page over to private.  If there's a dependency on the hypervisor doing
> > something, then it seems like we'll need to standardize that "something"
> > across hypervisors, lest we end up with per-hypervisor code in Linux to handle
> > this scenario.  And running SEV-SNP with multiple VMPLs probably makes it
> > even more complicated.
> > 
> > Kirill -- Same question about TDX.  Does making load_unaligned_zeropad()
> > work in a TDX VM depend on the hypervisor doing anything?  Or is the
> > behavior seen by the guest dependent only on architected behavior of
> > the TDX processor?
> 
> No, there's no active help from the hypervisor here.
> 
> Also, fwiw, the "architected behavior" here is really just the TDX
> module policy and _arguably_ the hardware Secure-EPT controlled by the
> TDX module.

Right. There's nothing VMM can do to change behaviour here. VMM can remove
private page, but it will lead to VM termination on access to the page,
but VMM controls VM lifecycle anyway.
  

On 5/26/23 05:02, Kirill A. Shutemov wrote:
> Touching privately mapped GPA that is not properly converted to private
> with MapGPA and accepted leads to unrecoverable exit to VMM.
> 
> load_unaligned_zeropad() can touch memory that is not owned by the
> caller, but just happened to next after the owned memory.
> This load_unaligned_zeropad() behaviour makes it important when kernel
> asks VMM to convert a GPA from shared to private or back. Kernel must
> never have a page mapped into direct mapping (and aliases) as private
> when the GPA is already converted to shared or when GPA is not yet
> converted to private.
> 
> guest.enc_status_change_prepare() called before adjusting direct mapping
> and therefore it is responsible for converting the memory to private.
> 
> guest.enc_status_change_finish() called after adjusting direct mapping
> and it converts the memory to shared.
> 
> It is okay to have a shared mapping of memory that is not converted
> properly. handle_mmio() knows how to deal with load_unaligned_zeropad()
> stepping on it.

Yeah, as other said, the changelog grammar here is a bit funky.  Can you
fix it up and resend, please?