From: Nicholas Piggin <npiggin@gmail.com> To: Fabiano Rosas <farosas@linux.ibm.com>, kvm-ppc@vger.kernel.org Cc: aik@ozlabs.ru, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size Date: Mon, 10 Jan 2022 17:38:24 +1000 [thread overview] Message-ID: <1641800177.nr6ngd1fot.astroid@bobo.none> (raw) In-Reply-To: <20220107210012.4091153-7-farosas@linux.ibm.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Nicholas Piggin <npiggin@gmail.com> To: Fabiano Rosas <farosas@linux.ibm.com>, kvm-ppc@vger.kernel.org Cc: aik@ozlabs.ru, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size Date: Mon, 10 Jan 2022 07:38:24 +0000 [thread overview] Message-ID: <1641800177.nr6ngd1fot.astroid@bobo.none> (raw) In-Reply-To: <20220107210012.4091153-7-farosas@linux.ibm.com> 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
next prev parent reply other threads:[~2022-01-10 7:39 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-07 20:58 [PATCH v3 0/6] KVM: PPC: MMIO fixes Fabiano Rosas 2022-01-07 21:00 ` Fabiano Rosas 2022-01-07 20:58 ` [PATCH v3 2/6] KVM: PPC: Fix vmx/vsx mixup in mmio emulation Fabiano Rosas 2022-01-07 21:00 ` Fabiano Rosas 2022-01-07 20:58 ` [PATCH v3 1/6] KVM: PPC: Book3S HV: Stop returning internal values to userspace Fabiano Rosas 2022-01-07 21:00 ` Fabiano Rosas 2022-01-07 20:59 ` [PATCH v3 5/6] KVM: PPC: mmio: Return to guest after emulation failure Fabiano Rosas 2022-01-07 21:00 ` Fabiano Rosas 2022-01-10 7:36 ` Nicholas Piggin 2022-01-10 7:36 ` Nicholas Piggin 2022-01-10 23:51 ` Alexey Kardashevskiy 2022-01-10 23:51 ` Alexey Kardashevskiy 2022-01-11 3:23 ` Nicholas Piggin 2022-01-11 3:23 ` Nicholas Piggin 2022-01-11 14:39 ` Fabiano Rosas 2022-01-11 14:39 ` Fabiano Rosas 2022-01-07 20:59 ` [PATCH v3 6/6] KVM: PPC: mmio: Reject instructions that access more than mmio.data size Fabiano Rosas 2022-01-07 21:00 ` Fabiano Rosas 2022-01-10 7:38 ` Nicholas Piggin [this message] 2022-01-10 7:38 ` Nicholas Piggin 2022-01-11 14:32 ` Fabiano Rosas 2022-01-11 14:32 ` Fabiano Rosas 2022-01-07 20:59 ` [PATCH v3 4/6] KVM: PPC: mmio: Queue interrupt at kvmppc_emulate_mmio Fabiano Rosas 2022-01-07 21:00 ` Fabiano Rosas 2022-01-10 3:20 ` Alexey Kardashevskiy 2022-01-10 3:20 ` Alexey Kardashevskiy 2022-01-10 5:29 ` Nicholas Piggin 2022-01-10 5:29 ` Nicholas Piggin 2022-01-07 20:59 ` [PATCH v3 3/6] KVM: PPC: Don't use pr_emerg when mmio emulation fails Fabiano Rosas 2022-01-07 21:00 ` Fabiano Rosas 2022-01-10 5:22 ` Nicholas Piggin 2022-01-10 5:22 ` Nicholas Piggin 2022-01-11 14:39 ` Fabiano Rosas 2022-01-11 14:39 ` Fabiano Rosas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1641800177.nr6ngd1fot.astroid@bobo.none \ --to=npiggin@gmail.com \ --cc=aik@ozlabs.ru \ --cc=farosas@linux.ibm.com \ --cc=kvm-ppc@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.