Re: [PATCH 06/23] MdePkg: Add TdxLib to wrap Tdx operations

["Erdem Aktas" <erdemaktas@google.com>]

On Thu, Aug 12, 2021 at 2:57 PM Min Xu <min.m.xu@intel.com> wrote:
>  - TdMaxVCpuNum    : Get the maximum number of virutal CPUs.
s/virutal/virtual

>  - TdVCpuNum       : Get the number of virtual CPUs. (In some case VMM may
>                      add more vCPU in runtime).
s/case/cases
How is this possible considering that once the TD is finalized, there
should not be any new vcpu added, right? Am I missing something here?


> +++ b/MdePkg/Library/TdxLib/X64/Tdcall.nasm
> @@ -0,0 +1,120 @@
> +;------------------------------------------------------------------------------
....
> +%macro tdcall_regs_preamble 2
is this even used in this file?


> +    ; Zero out unused (for standard TDVMCALL) registers to avoid leaking
> +    ; secrets to the VMM.
this is for TDCALL right, there is no leaking to the tdx module.


> +
> +    xor ebx, ebx
> +    xor esi, esi
> +    xor edi, edi
> +
> +    xor edx, edx
> +    xor ebp, ebp
zeroing only the lower 32bit values? why not the higher 32bit value if
leaking is the concern?


> +++ b/MdePkg/Library/TdxLib/X64/Tdvmcall.nasm

> +%define TDVMCALL_EXPOSE_REGS_MASK       0xffec
Should we expose only the minimum number of registers needed for the TDVMCALL?


>
> +%macro tdcall_regs_preamble 2
> +    mov rax, %1
> +
> +    mov ecx, %2
should not we make sure that the higher 32bit of RCX is 0? RCX [63:32]
are reserved and always need to be 0.

> +    ; R10 = 0 (standard TDVMCALL)
> +
> +    xor r10d, r10d
> +
> +    ; Zero out unused (for standard TDVMCALL) registers to avoid leaking
> +    ; secrets to the VMM.

Is not rcx the bitmap of the registers that will be exposed to VMM?
unused registers should be set 0 in the bitmap, why zeroing them?

> +
> +    xor ebx, ebx
> +    xor esi, esi
> +    xor edi, edi

> +    xor edx, edx
> +    xor ebp, ebp
if we are concerned about leaking some data, why xor only the lower 32bits?