linux-kernel - Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

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: <Zr1QJHj8W8L2BlvN@ls.amr.corp.intel.com>
Date: Wed, 14 Aug 2024 17:47:32 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: Yuan Yao <yuan.yao@...ux.intel.com>
Cc: Isaku Yamahata <isaku.yamahata@...el.com>,
	Rick Edgecombe <rick.p.edgecombe@...el.com>, seanjc@...gle.com,
	pbonzini@...hat.com, kvm@...r.kernel.org, kai.huang@...el.com,
	isaku.yamahata@...il.com, tony.lindgren@...ux.intel.com,
	xiaoyao.li@...el.com, linux-kernel@...r.kernel.org,
	Sean Christopherson <sean.j.christopherson@...el.com>,
	isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

On Wed, Aug 14, 2024 at 09:20:06AM +0800,
Yuan Yao <yuan.yao@...ux.intel.com> wrote:

> > > > +		ret = -EIO;
> > > > +		pr_tdx_error(TDH_VP_CREATE, err);
> > > > +		goto free_tdvpx;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < tdx_sysinfo_nr_tdcx_pages(); i++) {
> > > > +		va = __get_free_page(GFP_KERNEL_ACCOUNT);
> > > > +		if (!va) {
> > > > +			ret = -ENOMEM;
> > > > +			goto free_tdvpx;
> > >
> > > It's possible that some pages already added into TD by
> > > tdh_vp_addcx() below and they won't be handled by
> > > tdx_vcpu_free() if goto free_tdvpx here;
> >
> > Due to TDX TD state check, we can't free partially assigned TDCS pages.
> > TDX module seems to assume that TDH.VP.ADDCX() won't fail in the middle.
> 
> The already partially added TDCX pages are initialized by
> MOVDIR64 with the TD's private HKID in TDX module, the above
> 'goto free_tdvpx' frees them back to kernel directly w/o
> take back the ownership with shared HKID. This violates the
> rule that a page's ownership should be taken back with shared
> HKID before release to kernel if they were initialized by any
> private HKID before.
> 
> How about do tdh_vp_addcx() afer allocated all TDCX pages
> and give WARN_ON_ONCE() to the return value of
> tdh_vp_addcx() if the tdh_vp_addcx() won't fail except some
> BUG inside TDX module in our current usage ?

Yes, that makes sense.  Those error recovery paths need to be simplified.
-- 
Isaku Yamahata <isaku.yamahata@...el.com>