linux-kernel - Re: [RFC PATCH v3 08/11] KVM: Fix set_mem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZLHNx5jDjla2+4b0@google.com>
Date:   Fri, 14 Jul 2023 22:35:51 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Zhi Wang <zhi.wang.linux@...il.com>
Cc:     isaku.yamahata@...el.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
        Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
        Sagi Shahar <sagis@...gle.com>,
        David Matlack <dmatlack@...gle.com>,
        Kai Huang <kai.huang@...el.com>, chen.bo@...el.com,
        linux-coco@...ts.linux.dev,
        Chao Peng <chao.p.peng@...ux.intel.com>,
        Ackerley Tng <ackerleytng@...gle.com>,
        Vishal Annapurve <vannapurve@...gle.com>,
        Michael Roth <michael.roth@....com>,
        Yuan Yao <yuan.yao@...ux.intel.com>
Subject: Re: [RFC PATCH v3 08/11] KVM: Fix set_mem_attr ioctl when error case

On Fri, Jul 14, 2023, Zhi Wang wrote:
> On Thu, 13 Jul 2023 15:03:54 -0700
> Sean Christopherson <seanjc@...gle.com> wrote:
> > +       /*
> > +        * Reserve memory ahead of time to avoid having to deal with failures
> > +        * partway through setting the new attributes.
> > +        */
> > +       for (i = start; i < end; i++) {
> > +               r = xa_reserve(&kvm->mem_attr_array, i, GFP_KERNEL_ACCOUNT);
> > +               if (r)
> > +                       goto out_unlock;
> > +       }
> > +
> >         KVM_MMU_LOCK(kvm);
> >         kvm_mmu_invalidate_begin(kvm);
> >         kvm_mmu_invalidate_range_add(kvm, start, end);
> >         KVM_MMU_UNLOCK(kvm);
> >  
> >         for (i = start; i < end; i++) {
> > -               if (xa_err(xa_store(&kvm->mem_attr_array, i, entry,
> > -                                   GFP_KERNEL_ACCOUNT)))
> > +               r = xa_err(xa_store(&kvm->mem_attr_array, i, entry,
> > +                                   GFP_KERNEL_ACCOUNT));
> > +               if (KVM_BUG_ON(r, kvm))
> >                         break;
> >         }
> >
> 
> IIUC, If an error happenes here, we should bail out and call xa_release()?
> Or the code below (which is not shown here) still changes the memory attrs
> partially.

I'm pretty sure we want to continue on.  The VM is dead (killed by KVM_BUG_ON()),
so the attributes as seen by userspace and/or the VM don't matter.  What does
matter is that KVM's internal state is consistent, e.g. that KVM doesn't have
shared SPTEs while the attributes say a GFN is private.  That might not matter
for teardown, but I can't think of any reason not to tidy up.

And there can also be other ioctls() in flight.  KVM_REQ_VM_DEAD ensures vCPU
can't enter the guest, and vm->vm_dead ensures no new ioctls() cant start, but
neither of those guarantees there aren't other tasks doing KVM things.

Regardless, we definitely don't need to do xa_release().  The VM is dead and all
its memory will be reclaimed soon enough.  And there's no guarantee xa_release()
will actually be able to free anything, e.g. already processed entries won't be
freed, nor will any entries that existed _before_ the ioctl() was invoked.  Not
to mention that the xarray probably isn't consuming much memory, relatively
speaking.  I.e. in the majority of scenarios, it's likely preferable to get out
and destroy the VM asap.