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(¶ms, (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
