linux-kernel - Re: [PATCHv4 11/30] x86/tdx: Handle in-kernel MMIO

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] [day] [month] [year] [list]

Date:   Wed, 2 Mar 2022 16:42:45 +0300
From:   "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        luto@...nel.org, peterz@...radead.org,
        sathyanarayanan.kuppuswamy@...ux.intel.com, aarcange@...hat.com,
        ak@...ux.intel.com, dan.j.williams@...el.com, david@...hat.com,
        hpa@...or.com, jgross@...e.com, jmattson@...gle.com,
        joro@...tes.org, jpoimboe@...hat.com, knsathya@...nel.org,
        pbonzini@...hat.com, sdeep@...are.com, seanjc@...gle.com,
        tony.luck@...el.com, vkuznets@...hat.com, wanpengli@...cent.com,
        thomas.lendacky@....com, brijesh.singh@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCHv4 11/30] x86/tdx: Handle in-kernel MMIO

On Thu, Feb 24, 2022 at 12:11:54PM -0800, Dave Hansen wrote:
> 
> I found a few things lacking in that description.  How's that for a rewrite?

Looks great, thanks.
> > == Patching TDX drivers ==
> > 
> > Rather than touching the entire kernel, it might also be possible to
> > just go after drivers that use MMIO in TDX guests.  Right now, that's
> > limited only to virtio and some x86-specific drivers.
> > 
> > All virtio MMIO appears to be done through a single function, which
> > makes virtio eminently easy to patch. This will be implemented in the
> > future, removing the bulk of MMIO #VEs.
> 
> Given what is written here, this sounds like a great solution especially
> compared to all the instruction decoding nasiness.  What's wrong with it?

This will not cover non-virtio users. So #VE-based MMIO will remain as
fallback mechanism.

> > +	switch (mmio) {
> > +	case MMIO_WRITE:
> > +		memcpy(&val, reg, size);
> > +		return mmio_write(size, ve->gpa, val);
> > +	case MMIO_WRITE_IMM:
> > +		val = insn.immediate.value;
> > +		return mmio_write(size, ve->gpa, val);
> > +	case MMIO_READ:
> > +	case MMIO_READ_ZERO_EXTEND:
> > +	case MMIO_READ_SIGN_EXTEND:
> > +		break;
> > +	case MMIO_MOVS:
> > +	case MMIO_DECODE_FAILED:
> > +		return false;
> > +	default:
> > +		BUG();
> > +	}
> 
> Given the huge description above, it's borderline criminal to not
> discuss what could led to this BUG().

This bug actually covers "Unknown insn_decode_mmio() decode value" case.
I will add the comment.


> It could literally be some minor tweak in the compiler that changed a
> non-io.h-using MMIO access to get converted over to a instruction that
> can't be decoded.
> 
> Could we spend a few lines of comments to help out the future poor sod
> that sees "kernel bug at foo.c:1234"?  Maybe:
> 
> 	/*
> 	 * MMIO was accessed with an instruction that could not
> 	 * be decoded.  It was likely not using io.h helpers or
> 	 * accessed MMIO accidentally.
> 	 */

Thanks, I will use the comment for MMIO_DECODE_FAILED handling.

-- 
 Kirill A. Shutemov