Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Naresh Kamboju]

[Please ignore this email if it already reported ]

Following build errors noticed while building Linux next 20211018
with gcc-11 for i386 architecture.

i686-linux-gnu-ld: arch/x86/kvm/svm/sev.o: in function `sev_es_string_io':
sev.c:(.text+0x110f): undefined reference to `__udivdi3'
make[1]: *** [/builds/linux/Makefile:1247: vmlinux] Error 1
make[1]: Target '__all' not remade because of errors.
make: *** [Makefile:226: __sub-make] Error 2

Build config:
https://builds.tuxbuild.com/1zftLfjR2AyF4rsIfyUCnCTLKFs/config

Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>

meta data:
-----------
    git_describe: next-20211018
    git_repo: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
    git_sha: 60e8840126bdcb60bccef74c3f962742183c681f
    git_short_log: 60e8840126bd (\"Add linux-next specific files for 20211018\")
    kernel_version: 5.15.0-rc5
    target_arch: i386
    toolchain: gcc-11

steps to reproduce:
https://builds.tuxbuild.com/1zftLfjR2AyF4rsIfyUCnCTLKFs/tuxmake_reproducer.sh

--
Linaro LKFT
https://lkft.linaro.org

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Paolo Bonzini]

On 18/10/21 12:21, Naresh Kamboju wrote:
> [Please ignore this email if it already reported ]
> 
> Following build errors noticed while building Linux next 20211018
> with gcc-11 for i386 architecture.
> 
> i686-linux-gnu-ld: arch/x86/kvm/svm/sev.o: in function `sev_es_string_io':
> sev.c:(.text+0x110f): undefined reference to `__udivdi3'
> make[1]: *** [/builds/linux/Makefile:1247: vmlinux] Error 1
> make[1]: Target '__all' not remade because of errors.
> make: *** [Makefile:226: __sub-make] Error 2

Thank you very much, I have sent a simple fix of changing the variable 
to u32.

Paolo

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Paolo Bonzini]

On 25/10/21 03:31, Marc Orr wrote:
> I could be missing something, but I'm pretty sure that this is wrong.
> The GHCB spec says that `exit_info_2` is the `rep` count. Not the
> string length.
> 
> For example, given a `rep outsw` instruction, with `ECX` set to `8`,
> the rep count written into `SW_EXITINFO2` should be eight x86 words
> (i.e., 16 bytes) and the IO size should be one x86 word (i.e., 2
> bytes). In other words, the code was correct before this patch. This
> patch is incorrectly dividing the rep count by the IO size, causing
> the string IO to be truncated.

Then what's wrong is _also_ the call to setup_vmgexit_scratch, because 
that one definitely expects bytes:

                 scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT);

Paolo

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Marc Orr]

On Wed, Oct 13, 2021 at 9:56 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The size of the data in the scratch buffer is not divided by the size of
> each port I/O operation, so vcpu->arch.pio.count ends up being larger
> than it should be by a factor of size.
>
> Cc: stable@vger.kernel.org
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/sev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c36b5fe4c27c..e672493b5d8d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2583,7 +2583,7 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>                 return -EINVAL;
>
>         return kvm_sev_es_string_io(&svm->vcpu, size, port,
> -                                   svm->ghcb_sa, svm->ghcb_sa_len, in);
> +                                   svm->ghcb_sa, svm->ghcb_sa_len / size, in);
>  }
>
>  void sev_es_init_vmcb(struct vcpu_svm *svm)
> --
> 2.27.0
>
>

I could be missing something, but I'm pretty sure that this is wrong.
The GHCB spec says that `exit_info_2` is the `rep` count. Not the
string length.

For example, given a `rep outsw` instruction, with `ECX` set to `8`,
the rep count written into `SW_EXITINFO2` should be eight x86 words
(i.e., 16 bytes) and the IO size should be one x86 word (i.e., 2
bytes). In other words, the code was correct before this patch. This
patch is incorrectly dividing the rep count by the IO size, causing
the string IO to be truncated.

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Maxim Levitsky]

On Wed, 2021-10-13 at 12:56 -0400, Paolo Bonzini wrote:
> The size of the data in the scratch buffer is not divided by the size of
> each port I/O operation, so vcpu->arch.pio.count ends up being larger
> than it should be by a factor of size.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/sev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c36b5fe4c27c..e672493b5d8d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2583,7 +2583,7 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>  		return -EINVAL;
>  
>  	return kvm_sev_es_string_io(&svm->vcpu, size, port,
> -				    svm->ghcb_sa, svm->ghcb_sa_len, in);
> +				    svm->ghcb_sa, svm->ghcb_sa_len / size, in);
>  }
>  
>  void sev_es_init_vmcb(struct vcpu_svm *svm)

This ends in kvm_sev_es_ins/outs and both indeed expect count of operations which they pass to emulator_pio_{out|in}_emulated

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Tom Lendacky]

On 10/13/21 11:56 AM, Paolo Bonzini wrote:
> The size of the data in the scratch buffer is not divided by the size of
> each port I/O operation, so vcpu->arch.pio.count ends up being larger
> than it should be by a factor of size.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Ugh, can't believe I did that...

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   arch/x86/kvm/svm/sev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index c36b5fe4c27c..e672493b5d8d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2583,7 +2583,7 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>   		return -EINVAL;
>   
>   	return kvm_sev_es_string_io(&svm->vcpu, size, port,
> -				    svm->ghcb_sa, svm->ghcb_sa_len, in);
> +				    svm->ghcb_sa, svm->ghcb_sa_len / size, in);
>   }
>   
>   void sev_es_init_vmcb(struct vcpu_svm *svm)
> 

[PATCH 1/8] KVM: SEV-ES: fix length of string I/O

[Paolo Bonzini]

The size of the data in the scratch buffer is not divided by the size of
each port I/O operation, so vcpu->arch.pio.count ends up being larger
than it should be by a factor of size.

Cc: stable@vger.kernel.org
Fixes: 7ed9abfe8e9f ("KVM: SVM: Support string IO operations for an SEV-ES guest")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c36b5fe4c27c..e672493b5d8d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2583,7 +2583,7 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
 		return -EINVAL;
 
 	return kvm_sev_es_string_io(&svm->vcpu, size, port,
-				    svm->ghcb_sa, svm->ghcb_sa_len, in);
+				    svm->ghcb_sa, svm->ghcb_sa_len / size, in);
 }
 
 void sev_es_init_vmcb(struct vcpu_svm *svm)
-- 
2.27.0