Re: [PATCHv7 10/14] x86/mm: Avoid load_unaligned_zeropad() stepping into unaccepted memory

[Andy Lutomirski]

On Thu, Aug 11, 2022, at 4:26 AM, Borislav Petkov wrote:
> On Wed, Aug 03, 2022 at 07:02:31AM -0700, Dave Hansen wrote:
>> One other thing I remembered as I re-read my write up on this.
>> 
>> In the "new" mode, guests never get #VE's for unaccepted memory.  They
>> just exit to the host and can never be reentered.  They must be killed.
>
> Yeah, this is the part which I think is really silly.
>
> OSes, in their execution lifetime, can - erroneously or not - but it
> happens pretty often in real life, touch some unrelated memory. And this
> has never been a big deal - #PF, that's it.
>
> But now they don't even get a chance to correct their mistake - VMEXIT,
> die.
>
> load_unaligned_zeropad() is just one case.
>
> Imagine the user loads some buggy driver in the guest and that driver
> starts doing stray memory accesses through a wild pointer into the
> fields. Guest dies immediately.
>
> Dunno bit it all feels a bit too harsh and unfriendly to me.
>
> Sure, if that user is really unlucky, those stray accesses can kill
> his OS on baremetal too. So maybe you could argue here that such stray
> accesses are actually a good thing. :)
>
> All I know is, there should be a more resilient way to handle those.
>
>> In the "old" mode, I _believe_ that the guest always gets a #VE for
>> non-EPT-present memory.  The #VE is basically the same no matter if the
>> page is unaccepted or if the host goes out and makes a
>> previously-accepted page non-present.
>> 
>> One really nasty implication of this "old" mode is that the host can
>> remove *accepted* pages that are used in the syscall gap.  That means
>> that the #VE handler would need to be of the paranoid variety which
>> opens up all kinds of other fun.
>
> Yeah, I believe this needs to be dealt with anyway, for SNP at least.
> But on AMD it would simply cause an exception and it'll be handled in
> the #VC thing. And there's some ugly code to deal with the gap too.

I do not believe for a second that the “ugly” code in question is correct. Let’s please not go there for TDX.  The whole point of this thing is security — I would rather see a somewhat fragile situation than an exploit.

Now if the TD module could deliver an unrecoverable #MC instead of an impossible-to-handle #VE, maybe we could at least get a nice debug trace out?  Of course it’s not so easy to do anything with a debug trace that doesn’t break confidentiality.

So, Dave (and other Intel folks), here’s a different feature I think we will want: a secure way to get debug logs out of a TDX guest. Maybe the guest could have a way to send syslog lines to the TD module (via hypercall or a ring buffer) and the TD module could encrypt them against a secret that is only accessible to future boots of the same guest or to an attested outside key.  TDX already has an exceedingly flexible attestation mechanism — there is surely room for the guest config to include a public key that is used for encryption of exported logs.

(Yes, this is sort of doable in software, but the part where any kind of buffer actually gets saved anywhere if the guest goes kaboom requires either help from the TD module or a defined protocol with key negotiation involving the untrusted host.)

The model where the guest leaves a panic on the screen or serial console won’t work in TDX…

>
>>  * "Old" - #VE's can happen in the syscall gap
>>  * "New" - #VE's happen at better-defined times.  Unexpected ones are
>>    fatal.
>> 
>> There's a third option which I proposed but doesn't yet exist.  The TDX
>> module _could_ separate the behavior of unaccepted memory #VE's and
>> host-induced #VEs.  This way, we could use load_unaligned_zeropad() with
>> impunity and handle it in the #VE handler.  At the same time, the host
>> would not be allowed to remove accepted memory and cause problems in the
>> syscall gap.  Kinda the best of both worlds.
>
> I like that. This should've been the default from the get-go. Oh well,
> what's it called in English, hindsight is 20 20...?
>
>> But, I'm not sure how valuable that would be now that we have the
>> (admittedly squirrelly) code to avoid load_unaligned_zeropad() #VE's.
>
> I think you should push for the bestest solution and one day we can kill
> those ugly workarounds.
>
> Thx.
>
> -- 
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette