linux-kernel - Re: [PATCH 1/6] KVM: x86: inline kernel

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]

Message-ID: <YqJnKSpnPFZ5VsnL@google.com>
Date:   Thu, 9 Jun 2022 21:33:29 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH 1/6] KVM: x86: inline kernel_pio into its sole caller

On Wed, Jun 08, 2022, Paolo Bonzini wrote:
> The caller of kernel_pio already has arguments for most of what kernel_pio
> fishes out of vcpu->arch.pio.  This is the first step towards ensuring that
> vcpu->arch.pio.* is only used when exiting to userspace.
> 
> We can now also WARN if emulated PIO performs successful in-kernel iterations
> before having to fall back to userspace.  The code is not ready for that, and
> it should never happen.

Please avoid pronouns and state what patch does, not what "can" be done.  It's not
clear without reading the actual code whether "The code is not ready for that" means
"KVM is not ready to WARN" or "KVM is not ready to fall back to exiting userspace
if a

E.g.

  WARN if emulated PIO falls back to userspace after successfully handling
  one or more in-kernel iterations.  The port, size, and access type do not
  change, and KVM so it should be impossible for in-kernel PIO to fail on
  subsequent iterations.

That said, I don't think the above statement is true.  KVM is running with SRCU
protection, but the synchronize_srcu_expedited() in kvm_io_bus_unregister_dev()
only protects against use-after-free, it does not prevent two calls to
kvm_io_bus_read() from seeing different incarnations of kvm->buses.

And if I'm right, that could be exploited to create a buffer overrun due to doing
this memcpy with "data = <original data> + i * size".

	else
		memcpy(vcpu->arch.pio_data, data, size * count);

The existing code is arguably wrong too in that it will result in replaying PIO
accesses, but IMO userspace gets to keep the pieces if it unregisters a device
while vCPUs are running.
 
> No functional change intended.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/kvm/x86.c | 39 +++++++++++++++++----------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 79efdc19b4c8..2f9100f2564e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7415,37 +7415,32 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
>  	return emulator_write_emulated(ctxt, addr, new, bytes, exception);
>  }
>  
> -static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
> -{
> -	int r = 0, i;
> -
> -	for (i = 0; i < vcpu->arch.pio.count; i++) {
> -		if (vcpu->arch.pio.in)
> -			r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, vcpu->arch.pio.port,
> -					    vcpu->arch.pio.size, pd);
> -		else
> -			r = kvm_io_bus_write(vcpu, KVM_PIO_BUS,
> -					     vcpu->arch.pio.port, vcpu->arch.pio.size,
> -					     pd);
> -		if (r)
> -			break;
> -		pd += vcpu->arch.pio.size;
> -	}
> -	return r;
> -}
> -
>  static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
>  			       unsigned short port,
>  			       unsigned int count, bool in)
>  {
> +	void *data = vcpu->arch.pio_data;
> +	unsigned i;
> +	int r;
> +
>  	vcpu->arch.pio.port = port;
>  	vcpu->arch.pio.in = in;
> -	vcpu->arch.pio.count  = count;
> +	vcpu->arch.pio.count = count;
>  	vcpu->arch.pio.size = size;
>  
> -	if (!kernel_pio(vcpu, vcpu->arch.pio_data))
> -		return 1;
> +	for (i = 0; i < count; i++) {
> +		if (in)
> +			r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, port, size, data);
> +		else
> +			r = kvm_io_bus_write(vcpu, KVM_PIO_BUS, port, size, data);
> +		if (r)
> +			goto userspace_io;
> +		data += size;
> +	}
> +	return 1;
>  
> +userspace_io:
> +	WARN_ON(i != 0);
>  	vcpu->run->exit_reason = KVM_EXIT_IO;
>  	vcpu->run->io.direction = in ? KVM_EXIT_IO_IN : KVM_EXIT_IO_OUT;
>  	vcpu->run->io.size = size;
> -- 
> 2.31.1
> 
>