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.
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.
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.
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?