From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicholas Piggin Date: Mon, 10 Jan 2022 07:38:24 +0000 Subject: Re: [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size Message-Id: <1641800177.nr6ngd1fot.astroid@bobo.none> List-Id: References: <20220107210012.4091153-1-farosas@linux.ibm.com> <20220107210012.4091153-7-farosas@linux.ibm.com> In-Reply-To: <20220107210012.4091153-7-farosas@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Fabiano Rosas , kvm-ppc@vger.kernel.org Cc: aik@ozlabs.ru, linuxppc-dev@lists.ozlabs.org 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 > > Signed-off-by: Fabiano Rosas > Reviewed-by: Alexey Kardashevskiy > --- > 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