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?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
