| Message ID | 1455723260-23793-1-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 17, 2016 at 03:34:20PM +0000, Marc Zyngier wrote: > On an MMIO access, we always copy the on-stack buffer info > the shared "run" structure, even if this is a read access. > This ends up leaking up to 8 bytes of uninitialized memory > into userspace. I think it only leaks 'len' bytes to userspace ;) > > An obvious fix for this one is to only perform the copy if > this is an actual write. Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm/kvm/mmio.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c > index 7f33b20..0f6600f 100644 > --- a/arch/arm/kvm/mmio.c > +++ b/arch/arm/kvm/mmio.c > @@ -206,7 +206,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, > run->mmio.is_write = is_write; > run->mmio.phys_addr = fault_ipa; > run->mmio.len = len; > - memcpy(run->mmio.data, data_buf, len); > + if (is_write) > + memcpy(run->mmio.data, data_buf, len); > > if (!ret) { > /* We handled the access successfully in the kernel. */ > -- > 2.1.4 >
On 24/02/16 11:40, Christoffer Dall wrote: > On Wed, Feb 17, 2016 at 03:34:20PM +0000, Marc Zyngier wrote: >> On an MMIO access, we always copy the on-stack buffer info >> the shared "run" structure, even if this is a read access. >> This ends up leaking up to 8 bytes of uninitialized memory >> into userspace. > > I think it only leaks 'len' bytes to userspace ;) > >> >> An obvious fix for this one is to only perform the copy if >> this is an actual write. > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> Thanks. I've pushed this onto master, with a view of sending a PR to Paolo this evening (hopefully the last one for this cycle). M.
diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c index 7f33b20..0f6600f 100644 --- a/arch/arm/kvm/mmio.c +++ b/arch/arm/kvm/mmio.c @@ -206,7 +206,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, run->mmio.is_write = is_write; run->mmio.phys_addr = fault_ipa; run->mmio.len = len; - memcpy(run->mmio.data, data_buf, len); + if (is_write) + memcpy(run->mmio.data, data_buf, len); if (!ret) { /* We handled the access successfully in the kernel. */
On an MMIO access, we always copy the on-stack buffer info the shared "run" structure, even if this is a read access. This ends up leaking up to 8 bytes of uninitialized memory into userspace. An obvious fix for this one is to only perform the copy if this is an actual write. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm/kvm/mmio.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)