| Message ID | 20220107210012.4091153-7-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | KVM: PPC: MMIO fixes | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am: > The MMIO interface between the kernel and userspace uses a structure > that supports a maximum of 8-bytes of data. Instructions that access > more than that need to be emulated in parts. > > We currently don't have generic support for splitting the emulation in > parts and each set of instructions needs to be explicitly included. > > There's already an error message being printed when a load or store > exceeds the mmio.data buffer but we don't fail the emulation until > later at kvmppc_complete_mmio_load and even then we allow userspace to > make a partial copy of the data, which ends up overwriting some fields > of the mmio structure. > > This patch makes the emulation fail earlier at kvmppc_handle_load|store, > which will send a Program interrupt to the guest. This is better than > allowing the guest to proceed with partial data. > > Note that this was caught in a somewhat artificial scenario using > quadword instructions (lq/stq), there's no account of an actual guest > in the wild running instructions that are not properly emulated. > > (While here, fix the error message to check against 'bytes' and not > 'run->mmio.len' which at this point has an old value.) This looks good to me Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > arch/powerpc/kvm/powerpc.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index 56b0faab7a5f..a1643ca988e0 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -1246,7 +1246,8 @@ static int __kvmppc_handle_load(struct kvm_vcpu *vcpu, > > if (bytes > sizeof(run->mmio.data)) { > printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__, > - run->mmio.len); > + bytes); I wonder though this should probably be ratelimited, informational (or at least warning because it's a host message), and perhaps a bit more explanatory that it's a guest problem (or at least lack of host support for particular guest MMIO sizes). Thanks, Nick
Nicholas Piggin <npiggin@gmail.com> writes: > Excerpts from Fabiano Rosas's message of January 8, 2022 7:00 am: >> The MMIO interface between the kernel and userspace uses a structure >> that supports a maximum of 8-bytes of data. Instructions that access >> more than that need to be emulated in parts. >> >> We currently don't have generic support for splitting the emulation in >> parts and each set of instructions needs to be explicitly included. >> >> There's already an error message being printed when a load or store >> exceeds the mmio.data buffer but we don't fail the emulation until >> later at kvmppc_complete_mmio_load and even then we allow userspace to >> make a partial copy of the data, which ends up overwriting some fields >> of the mmio structure. >> >> This patch makes the emulation fail earlier at kvmppc_handle_load|store, >> which will send a Program interrupt to the guest. This is better than >> allowing the guest to proceed with partial data. >> >> Note that this was caught in a somewhat artificial scenario using >> quadword instructions (lq/stq), there's no account of an actual guest >> in the wild running instructions that are not properly emulated. >> >> (While here, fix the error message to check against 'bytes' and not >> 'run->mmio.len' which at this point has an old value.) > > This looks good to me > > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > >> >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> arch/powerpc/kvm/powerpc.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c >> index 56b0faab7a5f..a1643ca988e0 100644 >> --- a/arch/powerpc/kvm/powerpc.c >> +++ b/arch/powerpc/kvm/powerpc.c >> @@ -1246,7 +1246,8 @@ static int __kvmppc_handle_load(struct kvm_vcpu *vcpu, >> >> if (bytes > sizeof(run->mmio.data)) { >> printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__, >> - run->mmio.len); >> + bytes); > > I wonder though this should probably be ratelimited, informational (or > at least warning because it's a host message), and perhaps a bit more > explanatory that it's a guest problem (or at least lack of host support > for particular guest MMIO sizes). Yes, I'll ratelimit it an try to make it clear that this is something that happened inside the guest but it lacks support in KVM. Then hopefully people will tell to us if they ever need that support.
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 56b0faab7a5f..a1643ca988e0 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -1246,7 +1246,8 @@ static int __kvmppc_handle_load(struct kvm_vcpu *vcpu, if (bytes > sizeof(run->mmio.data)) { printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__, - run->mmio.len); + bytes); + return EMULATE_FAIL; } run->mmio.phys_addr = vcpu->arch.paddr_accessed; @@ -1335,7 +1336,8 @@ int kvmppc_handle_store(struct kvm_vcpu *vcpu, if (bytes > sizeof(run->mmio.data)) { printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__, - run->mmio.len); + bytes); + return EMULATE_FAIL; } run->mmio.phys_addr = vcpu->arch.paddr_accessed;