| Message ID | 20211223211528.3560711-4-farosas@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: PPC: Minor fixes | expand |
Excerpts from Fabiano Rosas's message of December 24, 2021 7:15 am: > We check against 'bytes' but print 'run->mmio.len' which at that point > has an old value. > > e.g. 16-byte load: > > before: > __kvmppc_handle_load: bad MMIO length: 8 > > now: > __kvmppc_handle_load: bad MMIO length: 16 > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> This patch fine, but in the case of overflow we continue anyway here. Can that overwrite some other memory in the kvm_run struct? This is familiar, maybe something Alexey has noticed in the past too? What was the consensus on fixing it? (at least it should have a comment if it's not a problem IMO) Thanks, Nick > --- > arch/powerpc/kvm/powerpc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index 793d42bd6c8f..7823207eb8f1 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -1246,7 +1246,7 @@ 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); > } > > run->mmio.phys_addr = vcpu->arch.paddr_accessed; > @@ -1335,7 +1335,7 @@ 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); > } > > run->mmio.phys_addr = vcpu->arch.paddr_accessed; > -- > 2.33.1 > >
Nicholas Piggin <npiggin@gmail.com> writes: > Excerpts from Fabiano Rosas's message of December 24, 2021 7:15 am: >> We check against 'bytes' but print 'run->mmio.len' which at that point >> has an old value. >> >> e.g. 16-byte load: >> >> before: >> __kvmppc_handle_load: bad MMIO length: 8 >> >> now: >> __kvmppc_handle_load: bad MMIO length: 16 >> >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > > This patch fine, but in the case of overflow we continue anyway here. > Can that overwrite some other memory in the kvm_run struct? I tested this and QEMU will indeed overwrite the subsequent fields of kvm_run. A `lq` on this data: mmio_test_data: .long 0xdeadbeef .long 0x8badf00d .long 0x1337c0de .long 0x01abcdef produces: __kvmppc_handle_load: bad MMIO length: 16 kvmppc_complete_mmio_load data: 0x8badf00ddeadbeef bad MMIO length: 322420958 <-- mmio.len got nuked But then we return from kvmppc_complete_mmio_load without writing to the registers. > > This is familiar, maybe something Alexey has noticed in the past too? > What was the consensus on fixing it? (at least it should have a comment > if it's not a problem IMO) My plan was to just add quadword support. And whatever else might missing. But I got sidetracked with how to test this so I'm just now coming back to it. Perhaps a more immediate fix is needed before that? We could block loads and stores larger than 8 bytes earlier at kvmppc_emulate_loadstore for instance.
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 793d42bd6c8f..7823207eb8f1 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -1246,7 +1246,7 @@ 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); } run->mmio.phys_addr = vcpu->arch.paddr_accessed; @@ -1335,7 +1335,7 @@ 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); } run->mmio.phys_addr = vcpu->arch.paddr_accessed;
We check against 'bytes' but print 'run->mmio.len' which at that point has an old value. e.g. 16-byte load: before: __kvmppc_handle_load: bad MMIO length: 8 now: __kvmppc_handle_load: bad MMIO length: 16 Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- arch/powerpc/kvm/powerpc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)