linux-kernel - Re: [PATCH] x86/tdx: Handle load_unaligned

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]

Date:   Thu, 19 May 2022 21:19:58 +0300
From:   "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Dave Hansen <dave.hansen@...el.com>, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, luto@...nel.org,
        peterz@...radead.org, sathyanarayanan.kuppuswamy@...ux.intel.com,
        ak@...ux.intel.com, dan.j.williams@...el.com, david@...hat.com,
        hpa@...or.com, thomas.lendacky@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/tdx: Handle load_unaligned_zeropad() page-cross to a
 shared page

On Tue, May 17, 2022 at 10:52:48PM +0000, Sean Christopherson wrote:
> On Tue, May 17, 2022, Sean Christopherson wrote:
> > On Tue, May 17, 2022, Dave Hansen wrote:
> > > On 5/17/22 13:17, Kirill A. Shutemov wrote:
> > > >>> Given that we had to adjust IP in handle_mmio() anyway, do you still think
> > > >>> "ve->instr_len = 0;" is wrong? I dislike ip_adjusted more.
> > > >> Something is wrong about it.
> > > >>
> > > >> You could call it 've->instr_bytes_to_handle' or something. Then it
> > > >> makes actual logical sense when you handle it to zero it out.  I just
> > > >> want it to be more explicit when the upper levels need to do something.
> > > >>
> > > >> Does ve->instr_len==0 both when the TDX module isn't providing
> > > >> instruction sizes *and* when no handling is necessary?  That seems like
> > > >> an unfortunate logical multiplexing of 0.
> > > > For EPT violation, ve->instr_len has *something* (not zero) that doesn't
> > > > match the actual instruction size. I dig out that it is filled with data
> > > > from VMREAD(0x440C), but I don't know where is the ultimate origin of the
> > > > data.
> > > 
> > > The SDM has a breakdown:
> > > 
> > > 	27.2.5 Information for VM Exits Due to Instruction Execution
> > > 
> > > I didn't realize it came from VMREAD.  I guess I assumed it came from
> > > some TDX module magic.  Silly me.
> > > 
> > > The SDM makes it sound like we should be more judicious about using
> > > 've->instr_len' though.  "All VM exits other than those listed in the
> > > above items leave this field undefined."  Looking over
> > > virt_exception_kernel(), we've got five cases from CPU instructions that
> > > cause unconditional VMEXITs:
> 
> Ideally, what the SDM says wouldn't matter at all.  The TDX module spec really
> should be the authorative source in this case, but it just punts to the SDM:
> 
>   The 32-bit value that would have been saved into the VMCS as VM-exit instruction
>   length if a legacy VM exit had occurred instead of the virtualization exception.
> 
> Even if the TDX spec wants to punt to the SDM, it would save a lot of headache and
> SDM reading if it also said something to the effect of:
> 
>   The INSTRUCTION_LENGTH and INSTRUCTION_INFORMATION fields are valid for all
>   #VEs injected by the Intel TDX Module.  The fields are undefined for #VEs
>   injected by the CPU due to EPT Violations.

I initiated update to the spec, but it will take time.

-- 
 Kirill A. Shutemov