Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <57c77407-d0bc-5e74-490b-f604b4f97691@amd.com>
Date:   Wed, 23 Nov 2022 12:32:19 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH Part2 v6 14/49] crypto: ccp: Handle the legacy TMR
 allocation when SNP is enabled
Content-Language: en-US
To:     Borislav Petkov <bp@alien8.de>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        linux-coco@lists.linux.dev, linux-mm@kvack.org,
        linux-crypto@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com,
        jroedel@suse.de, thomas.lendacky@amd.com, hpa@zytor.com,
        ardb@kernel.org, pbonzini@redhat.com, seanjc@google.com,
        vkuznets@redhat.com, jmattson@google.com, luto@kernel.org,
        dave.hansen@linux.intel.com, slp@redhat.com, pgonda@google.com,
        peterz@infradead.org, srinivas.pandruvada@linux.intel.com,
        rientjes@google.com, dovmurik@linux.ibm.com, tobin@ibm.com,
        michael.roth@amd.com, vbabka@suse.cz, kirill@shutemov.name,
        ak@linux.intel.com, tony.luck@intel.com, marcorr@google.com,
        sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com,
        dgilbert@redhat.com, jarkko@kernel.org
References: <3a51840f6a80c87b39632dc728dbd9b5dd444cd7.1655761627.git.ashish.kalra@amd.com>
 <Y0grhk1sq2tf/tUl@zn.tnic> <380c9748-1c86-4763-ea18-b884280a3b60@amd.com>
 <b712bc81-4446-9be4-fd59-d08981d13475@amd.com> <Y3qdTuZQoDZxUgbw@zn.tnic>
 <cffed3c2-55a9-bdd3-3b8a-82b2050a64af@amd.com> <Y3yhthJTIWqjjAPK@zn.tnic>
 <d85bf488-9050-13d6-a23b-1440a4df4c81@amd.com> <Y3yoB015qCpbnUzl@zn.tnic>
 <13bd73b6-592c-66c4-cd42-0913380da745@amd.com> <Y34GuNXaa3csthJY@zn.tnic>
From:   "Kalra, Ashish" <ashish.kalra@amd.com>
In-Reply-To: <Y34GuNXaa3csthJY@zn.tnic>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TjRsOW1xMDlzQXVYZmY2OHNud0JEK056dnQ4UnNUNCsySXpNajJFNFhKRklU?=
 =?utf-8?B?M0tON1lVay9wR1JmZkIyamJQRXBrdElueUowTE5kZ1dBSzZpY3ZrY2xseTA2?=
 =?utf-8?B?ZGNLcXZWU1J2dHBNUGlza2IyMmlsbGtQY0FJN0MvMHp6bVZWRkZxY2lkT09t?=
 =?utf-8?B?aVRtRVVVZDhkZERRUGRVaXRWUTdLOHJKNmpETXl6ZGUzNVlUQVE5UmN0YmZD?=
 =?utf-8?B?anFOa3liVjlLdHlROCszcSt3S0hWZUFnbnBGd2VuL1dnSWM5cTROMUQzY0NJ?=
 =?utf-8?B?S2p0b1lTQm44VTY4MjFkSlM0YWFXSitwMUNrNXNZTFRROGdRVUhHU1cvMDBG?=
 =?utf-8?B?MFJxTDdZY2dtZEx3cDY3NzVUbUNGYklRRWJVZmdaSzIyUjFPVnlSc3pBcjFp?=
 =?utf-8?B?VEZYYklRSUNBMkdmaDZwNWY1ejJPQytrS1Rib2QrS09xUkh3SEd0NVB4Tity?=
 =?utf-8?B?RE1LOFZreUNrTVBwNE5JSEdYemZrcm5MS1BGNUJpQnkzT2ZIOEM4by9XYUh3?=
 =?utf-8?B?OHF0dHU0TWExWkdKckwreUFDejM4ak9IRnVVeTYrbFc4emw0N09XVEVGcWFl?=
 =?utf-8?B?SWUvOURWejA2bnVWRFBQVGlwUitvem5OdVVmTm5YeVRtMmpFN0U5OGxjWkZU?=
 =?utf-8?B?M0hLa3ZBdTlUNlUxdHg4cFA2dmlESVcvNmplQTdrb080TFNUYWhKaXB1bGFi?=
 =?utf-8?B?QkozNCtSZkhwNlNIWnRFQm92Tm82SklzMHlrVDRKWGxES2psZFpiQWZramY4?=
 =?utf-8?B?NXV4Q0ZyeHZ6TnN1M1N1b2VPbStkQ0JvVlUzTnhMSmpsSjFQcmU5UXAxQmFx?=
 =?utf-8?B?NzNwdzhpZFlZdVcvbXZ2Wm1QdUtxZjNySlU4dlc3ZFNla3lYbGh0Z1ZNSi9o?=
 =?utf-8?B?UU9nd0NsQVphRng1d294aDJSMTk3YWtlRjB3a1FFZVh4QWt1OHVjRmhlWEEz?=
 =?utf-8?B?VUgrblNYTFFkY2hDeXp6eW1VUkNZYTkvNXVTVlhiclJFdVNNOGpScWw1ODNn?=
 =?utf-8?B?NjdiVk93M0w2VHRWUTVmWGNoTnVGSmNiTFo1dkluM2JIbmhaNHcvMVA5RVdI?=
 =?utf-8?B?LysvbHFSdEdkYk5BK0VUWm50TDdYTEwrV0xvbFRna2xGQnN2TU5Ba0FoMTRP?=
 =?utf-8?B?TlV4OGowVm5nM3JuSUlHd1pxRmF5UHA0Q2JyRG5JT0ovOHhDcnZBNHdLeXlZ?=
 =?utf-8?B?UFl4cGVOaDNEK3RlSUdDeFV2NG5OWTFza3h4WjE0Mmd5ZUNUN0JBVzJmTTlh?=
 =?utf-8?B?Nlh1OVo3aVY1SVA1ZEFrbnhDMFNvMUVhb24wUjdRQmxKVGhncDg3blErL0o0?=
 =?utf-8?B?RUhVVEg4VHlnbExwUWxDMWNocldtOEFYeDRLZWF1Mmt3ZzdlL1lGK2hrUEkx?=
 =?utf-8?B?MEZhYzVtRWR6dTNnUHpBMzFxZU9Ua09BblErT3ZiRFA3UC85Q0o4RjVOTi80?=
 =?utf-8?B?UnlWOUdQVVNkdVlpd2d2VkJicGpjSFZUanA1Wm9YTWgvLytVODl0RHBrbk80?=
 =?utf-8?B?OE0zbm1SeXJJcFpVc2dTNzljbml5SlhaOGplNTYvNGw3WFNRakN5NHY3aC9R?=
 =?utf-8?B?LzNMQWtLWGc1UGc2UjRyRStjRFJuc09INXdUbG9jTDB1MForT2F3Y09VR0J0?=
 =?utf-8?B?UFZCaFc4Y0JsK29IOElPZVhvNWM1R0tIZWpicXI1bTcrSCsrTXVoNHQ1dklD?=
 =?utf-8?B?ZStqR1Z1aVpsTXBSZkIvNm5OT2c5eEc3RjJ0Smt2Rld1Z1JlTzh1ZjZEU0c0?=
 =?utf-8?B?bjg0ejFXNG9sbllhRVMya25ZUTg4K2daeTkwOUxjNVVvTmdQSEtYL2RYV1ZB?=
 =?utf-8?B?QnBxZHBZS0tzRTVPYzhBUStsZHhHdUx3Y2xVSWNhL3BDSURoUUJpTFpKK0VD?=
 =?utf-8?B?alAxYnM5OE0xOVlqUXUyTE5WNFlpd21yQlFNdDlBZnBNTXFDRzNsM290eW1x?=
 =?utf-8?B?emFpcURBMjdkdWtaK0ZZRUxHUkpJeE1yMkl5ZFMzdHR0OGVHVEd2VUE4ZEZk?=
 =?utf-8?B?QlNwTnQ5bC9pOEdSZ1hlZVlFT1pCaHIzQy9lQnpUSXpFRm01VEY2TDJTTkE3?=
 =?utf-8?B?dmhsYVZLcjRBdCsvdGRaYktabW9CVnNEaGVhYm1DT21Vb0JpSHJ6R1phbUlM?=
 =?utf-8?Q?wk3aLGc7L7+kH+FEAd7kWDAFz?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7abe33c1-c0db-4fbb-1bbc-08dacd8110e9
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2767.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Nov 2022 18:32:24.6252
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: X00k21BPys6c6hCICrYwGLbNxPBulQisFoDNeCuPO3dOGTerQvWOT4+h9BGqaYtqT50u1X6QUia8AZhXYB9ihA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB8148
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

Hello Boris,

On 11/23/2022 5:40 AM, Borislav Petkov wrote:
> On Tue, Nov 22, 2022 at 05:44:47AM -0600, Kalra, Ashish wrote:
>> It is important to note that if invalid address/len are supplied, the
>> failure will happen at the initial stage itself of transitioning these pages
>> to firmware state.
> 
> /me goes and checks out your v6 tree based on 5.18.
> 
> Lemme choose one:
> 
> static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
> {
> 	...
> 
>          inpages = sev_pin_memory(kvm, params.uaddr, params.len, &npages, 1);
> 
> 	...
> 
>          for (i = 0; i < npages; i++) {
>                  pfn = page_to_pfn(inpages[i]);
> 
> 		...
> 
>                  ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_LAUNCH_UPDATE, &data, error);
>                  if (ret) {
>                          /*
>                           * If the command failed then need to reclaim the page.
>                           */
>                          snp_page_reclaim(pfn);
> 
> and here it would leak the pages if it cannot reclaim them.
> 
> Now how did you get those?
> 
> Through params.uaddr and params.len which come from userspace:
> 
>          if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
>                  return -EFAULT;
> 
> 
> Now, think about it, can userspace be trusted?
> 
> Exactly.
> 
> Yeah, yeah, I see it does is_hva_registered() but userspace can just as
> well supply the wrong region which fits.

Yes, that's right.

Also, before sev_issue_cmd() above, there is a call to 
rmp_make_private() to make these pages transition to firmware state 
before we issue the LAUNCH_UPDATE command as below:

            ret = rmp_make_private(pfn, gfn << PAGE_SHIFT, level,
				  sev_get_asid(kvm), true);
                 if (ret) {
                         ret = -EFAULT;
                         goto e_unpin;

                 }
...
  	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_LAUNCH_UPDATE,
			      &data, error);

So in case, the userspace provided an invalid/incorrect range, this
transition would have failed and there would not have been a need to do 
any reclaim, so there are no pages leaked here.

This is also the reason why we need to reclaim pages if the subsequent 
LAUNCH_UPDATE command fails, as now the pages are in F/W state because 
of the rmp_make_private() call and they are now unsafe to be used by the 
host.

> 
>> In such a case the kernel panic is justifiable,
> 
> So userspace can supply whatever it wants and you'd panic?
> 
> You surely don't mean that.
> 

No, we don't want to do that.

>> but again if incorrect addresses are supplied, the failure will happen
>> at the initial stage of transitioning these pages to firmware state
>> and there is no need to reclaim.

This is the case i mentioned above, rmp_make_private() before the 
firmware command is the initial stage of transitioning the pages to 
firmware state before issuing the firmware command.

> 
> See above.
> 
>> Or, otherwise dump a warning and let the pages not be freed/returned
>> back to the page allocator.
>>
>> It is either innocent pages or kernel panic or an innocent host
>> process crash (these are the choices to make).
> 
> No, it is make the kernel as resilient as possible. Which means, no
> panic, add the pages to a not-to-be-used-anymore list and scream loudly
> with warning messages when it must leak pages so that people can fix the
> issue.
> 
> Ok?
> 

Right, yes, i totally agree.

So now we are adding these pages to an internal not-to-be-used-anymore 
list and printing warnings and ensuring no panics as we don't allow 
these pages to be released back to the page allocator.

Thanks,
Ashish