From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Sun, 22 Oct 2017 09:44:18 +0200 From: Christoffer Dall Message-ID: <20171022074418.GA3805@cbox> References: <20171020232525.7387-1-pbonzini@redhat.com> <20171021184545.2497-1-christoffer.dall@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: [kernel-hardening] Re: [PATCH] KVM: arm/arm64: Allow usercopy to vcpu->arch.ctxt and arm64 debug To: Kees Cook Cc: Christoffer Dall , Paolo Bonzini , kvmarm@lists.cs.columbia.edu, "linux-arm-kernel@lists.infradead.org" , KVM , "kernel-hardening@lists.openwall.com" , Radim =?utf-8?B?S3LEjW3DocWZ?= , Marc Zyngier List-ID: On Sat, Oct 21, 2017 at 08:06:10PM -0700, Kees Cook wrote: > On Sat, Oct 21, 2017 at 11:45 AM, Christoffer Dall > wrote: > > We do direct useraccess copying to the kvm_cpu_context structure > > embedded in the kvm_vcpu_arch structure, and to the vcpu debug register > > state. Everything else (timer, PMU, vgic) goes through a temporary > > indirection. > > Are these copies done with a dynamic size? The normal way these get > whitelisted is via builtin_const sizes on the copy. Looking at > KVM_REG_SIZE(), though, it seems that would be a dynamic calculation. > It's super confusing, but it's actually static. We can only get to thee functions via kvm_arm_sys_reg_get_reg() and kvm_arm_sys_reg_set_reg(), and they both do if (KVM_REG_SIZE(reg->id) != sizeof(__u64))" return -ENOENT; So this is always a u64 copy. However, I think it's much clearer if I rewrite these to use get_user() and put_user(). v2 incoming. > > Fixing all accesses to kvm_cpu_context is massively invasive, and we'd > > like to avoid that, so we tell kvm_init_usercopy to whitelist accesses > > to out context structure. > > > > The debug system register accesses on arm64 are modified to work through > > an indirection instead. > > > > Cc: kernel-hardening@lists.openwall.com > > Cc: Kees Cook > > Cc: Paolo Bonzini > > Cc: Radim Krčmář > > Cc: Marc Zyngier > > Signed-off-by: Christoffer Dall > > --- > > This fixes KVM/ARM on today's linux next with CONFIG_HARDENED_USERCOPY. > > > > The patch is based on linux-next plus Paolo's x86 patch which introduces > > kvm_init_usercopy. Not sure how this needs to get merged, but it would > > potentially make sense for Paolo to put together a set of the patches > > needed for this. > > I was planning to carry Paolo's patches, and I can take this one too. Sounds good to me. > If this poses a problem, then I could just do a two-phase commit of > the whitelisting code, leaving the very last commit (which enables the > defense for anything not yet whitelisted), until the KVM trees land. > > What's preferred? Assuming there's an ack from Marc Zyngier on v2 of this patch, I prefer you just take them as part of your series. > > Thanks for looking at this! > No problem, -Christoffer From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH] KVM: arm/arm64: Allow usercopy to vcpu->arch.ctxt and arm64 debug Date: Sun, 22 Oct 2017 09:44:18 +0200 Message-ID: <20171022074418.GA3805@cbox> References: <20171020232525.7387-1-pbonzini@redhat.com> <20171021184545.2497-1-christoffer.dall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 1B13749D25 for ; Sun, 22 Oct 2017 03:43:12 -0400 (EDT) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id OAoMoOtBLOz9 for ; Sun, 22 Oct 2017 03:43:10 -0400 (EDT) Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 02F6E40638 for ; Sun, 22 Oct 2017 03:43:09 -0400 (EDT) Received: by mail-wm0-f66.google.com with SMTP id r68so4457526wmr.3 for ; Sun, 22 Oct 2017 00:44:21 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Kees Cook Cc: KVM , "kernel-hardening@lists.openwall.com" , Marc Zyngier , Paolo Bonzini , kvmarm@lists.cs.columbia.edu, "linux-arm-kernel@lists.infradead.org" List-Id: kvmarm@lists.cs.columbia.edu T24gU2F0LCBPY3QgMjEsIDIwMTcgYXQgMDg6MDY6MTBQTSAtMDcwMCwgS2VlcyBDb29rIHdyb3Rl Ogo+IE9uIFNhdCwgT2N0IDIxLCAyMDE3IGF0IDExOjQ1IEFNLCBDaHJpc3RvZmZlciBEYWxsCj4g PGNocmlzdG9mZmVyLmRhbGxAbGluYXJvLm9yZz4gd3JvdGU6Cj4gPiBXZSBkbyBkaXJlY3QgdXNl cmFjY2VzcyBjb3B5aW5nIHRvIHRoZSBrdm1fY3B1X2NvbnRleHQgc3RydWN0dXJlCj4gPiBlbWJl ZGRlZCBpbiB0aGUga3ZtX3ZjcHVfYXJjaCBzdHJ1Y3R1cmUsIGFuZCB0byB0aGUgdmNwdSBkZWJ1 ZyByZWdpc3Rlcgo+ID4gc3RhdGUuICBFdmVyeXRoaW5nIGVsc2UgKHRpbWVyLCBQTVUsIHZnaWMp IGdvZXMgdGhyb3VnaCBhIHRlbXBvcmFyeQo+ID4gaW5kaXJlY3Rpb24uCj4gCj4gQXJlIHRoZXNl IGNvcGllcyBkb25lIHdpdGggYSBkeW5hbWljIHNpemU/IFRoZSBub3JtYWwgd2F5IHRoZXNlIGdl dAo+IHdoaXRlbGlzdGVkIGlzIHZpYSBidWlsdGluX2NvbnN0IHNpemVzIG9uIHRoZSBjb3B5LiBM b29raW5nIGF0Cj4gS1ZNX1JFR19TSVpFKCksIHRob3VnaCwgaXQgc2VlbXMgdGhhdCB3b3VsZCBi ZSBhIGR5bmFtaWMgY2FsY3VsYXRpb24uCj4gCgpJdCdzIHN1cGVyIGNvbmZ1c2luZywgYnV0IGl0 J3MgYWN0dWFsbHkgc3RhdGljLgoKV2UgY2FuIG9ubHkgZ2V0IHRvIHRoZWUgZnVuY3Rpb25zIHZp YSBrdm1fYXJtX3N5c19yZWdfZ2V0X3JlZygpIGFuZAprdm1fYXJtX3N5c19yZWdfc2V0X3JlZygp LCBhbmQgdGhleSBib3RoIGRvCgoJaWYgKEtWTV9SRUdfU0laRShyZWctPmlkKSAhPSBzaXplb2Yo X191NjQpKSIKCQlyZXR1cm4gLUVOT0VOVDsKClNvIHRoaXMgaXMgYWx3YXlzIGEgdTY0IGNvcHku ICBIb3dldmVyLCBJIHRoaW5rIGl0J3MgbXVjaCBjbGVhcmVyIGlmIEkKcmV3cml0ZSB0aGVzZSB0 byB1c2UgZ2V0X3VzZXIoKSBhbmQgcHV0X3VzZXIoKS4gIHYyIGluY29taW5nLgoKPiA+IEZpeGlu ZyBhbGwgYWNjZXNzZXMgdG8ga3ZtX2NwdV9jb250ZXh0IGlzIG1hc3NpdmVseSBpbnZhc2l2ZSwg YW5kIHdlJ2QKPiA+IGxpa2UgdG8gYXZvaWQgdGhhdCwgc28gd2UgdGVsbCBrdm1faW5pdF91c2Vy Y29weSB0byB3aGl0ZWxpc3QgYWNjZXNzZXMKPiA+IHRvIG91dCBjb250ZXh0IHN0cnVjdHVyZS4K PiA+Cj4gPiBUaGUgZGVidWcgc3lzdGVtIHJlZ2lzdGVyIGFjY2Vzc2VzIG9uIGFybTY0IGFyZSBt b2RpZmllZCB0byB3b3JrIHRocm91Z2gKPiA+IGFuIGluZGlyZWN0aW9uIGluc3RlYWQuCj4gPgo+ ID4gQ2M6IGtlcm5lbC1oYXJkZW5pbmdAbGlzdHMub3BlbndhbGwuY29tCj4gPiBDYzogS2VlcyBD b29rIDxrZWVzY29va0BjaHJvbWl1bS5vcmc+Cj4gPiBDYzogUGFvbG8gQm9uemluaSA8cGJvbnpp bmlAcmVkaGF0LmNvbT4KPiA+IENjOiBSYWRpbSBLcsSNbcOhxZkgPHJrcmNtYXJAcmVkaGF0LmNv bT4KPiA+IENjOiBNYXJjIFp5bmdpZXIgPG1hcmMuenluZ2llckBhcm0uY29tPgo+ID4gU2lnbmVk LW9mZi1ieTogQ2hyaXN0b2ZmZXIgRGFsbCA8Y2hyaXN0b2ZmZXIuZGFsbEBsaW5hcm8ub3JnPgo+ ID4gLS0tCj4gPiBUaGlzIGZpeGVzIEtWTS9BUk0gb24gdG9kYXkncyBsaW51eCBuZXh0IHdpdGgg Q09ORklHX0hBUkRFTkVEX1VTRVJDT1BZLgo+ID4KPiA+IFRoZSBwYXRjaCBpcyBiYXNlZCBvbiBs aW51eC1uZXh0IHBsdXMgUGFvbG8ncyB4ODYgcGF0Y2ggd2hpY2ggaW50cm9kdWNlcwo+ID4ga3Zt X2luaXRfdXNlcmNvcHkuICBOb3Qgc3VyZSBob3cgdGhpcyBuZWVkcyB0byBnZXQgbWVyZ2VkLCBi dXQgaXQgd291bGQKPiA+IHBvdGVudGlhbGx5IG1ha2Ugc2Vuc2UgZm9yIFBhb2xvIHRvIHB1dCB0 b2dldGhlciBhIHNldCBvZiB0aGUgcGF0Y2hlcwo+ID4gbmVlZGVkIGZvciB0aGlzLgo+IAo+IEkg d2FzIHBsYW5uaW5nIHRvIGNhcnJ5IFBhb2xvJ3MgcGF0Y2hlcywgYW5kIEkgY2FuIHRha2UgdGhp cyBvbmUgdG9vLgoKU291bmRzIGdvb2QgdG8gbWUuCgo+IElmIHRoaXMgcG9zZXMgYSBwcm9ibGVt LCB0aGVuIEkgY291bGQganVzdCBkbyBhIHR3by1waGFzZSBjb21taXQgb2YKPiB0aGUgd2hpdGVs aXN0aW5nIGNvZGUsIGxlYXZpbmcgdGhlIHZlcnkgbGFzdCBjb21taXQgKHdoaWNoIGVuYWJsZXMg dGhlCj4gZGVmZW5zZSBmb3IgYW55dGhpbmcgbm90IHlldCB3aGl0ZWxpc3RlZCksIHVudGlsIHRo ZSBLVk0gdHJlZXMgbGFuZC4KPiAKPiBXaGF0J3MgcHJlZmVycmVkPwoKQXNzdW1pbmcgdGhlcmUn cyBhbiBhY2sgZnJvbSBNYXJjIFp5bmdpZXIgb24gdjIgb2YgdGhpcyBwYXRjaCwgSSBwcmVmZXIK eW91IGp1c3QgdGFrZSB0aGVtIGFzIHBhcnQgb2YgeW91ciBzZXJpZXMuCgo+IAo+IFRoYW5rcyBm b3IgbG9va2luZyBhdCB0aGlzIQo+IApObyBwcm9ibGVtLAotQ2hyaXN0b2ZmZXIKX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18Ka3ZtYXJtIG1haWxpbmcgbGlz dAprdm1hcm1AbGlzdHMuY3MuY29sdW1iaWEuZWR1Cmh0dHBzOi8vbGlzdHMuY3MuY29sdW1iaWEu ZWR1L21haWxtYW4vbGlzdGluZm8va3ZtYXJtCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: cdall@linaro.org (Christoffer Dall) Date: Sun, 22 Oct 2017 09:44:18 +0200 Subject: [PATCH] KVM: arm/arm64: Allow usercopy to vcpu->arch.ctxt and arm64 debug In-Reply-To: References: <20171020232525.7387-1-pbonzini@redhat.com> <20171021184545.2497-1-christoffer.dall@linaro.org> Message-ID: <20171022074418.GA3805@cbox> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sat, Oct 21, 2017 at 08:06:10PM -0700, Kees Cook wrote: > On Sat, Oct 21, 2017 at 11:45 AM, Christoffer Dall > wrote: > > We do direct useraccess copying to the kvm_cpu_context structure > > embedded in the kvm_vcpu_arch structure, and to the vcpu debug register > > state. Everything else (timer, PMU, vgic) goes through a temporary > > indirection. > > Are these copies done with a dynamic size? The normal way these get > whitelisted is via builtin_const sizes on the copy. Looking at > KVM_REG_SIZE(), though, it seems that would be a dynamic calculation. > It's super confusing, but it's actually static. We can only get to thee functions via kvm_arm_sys_reg_get_reg() and kvm_arm_sys_reg_set_reg(), and they both do if (KVM_REG_SIZE(reg->id) != sizeof(__u64))" return -ENOENT; So this is always a u64 copy. However, I think it's much clearer if I rewrite these to use get_user() and put_user(). v2 incoming. > > Fixing all accesses to kvm_cpu_context is massively invasive, and we'd > > like to avoid that, so we tell kvm_init_usercopy to whitelist accesses > > to out context structure. > > > > The debug system register accesses on arm64 are modified to work through > > an indirection instead. > > > > Cc: kernel-hardening at lists.openwall.com > > Cc: Kees Cook > > Cc: Paolo Bonzini > > Cc: Radim Kr?m?? > > Cc: Marc Zyngier > > Signed-off-by: Christoffer Dall > > --- > > This fixes KVM/ARM on today's linux next with CONFIG_HARDENED_USERCOPY. > > > > The patch is based on linux-next plus Paolo's x86 patch which introduces > > kvm_init_usercopy. Not sure how this needs to get merged, but it would > > potentially make sense for Paolo to put together a set of the patches > > needed for this. > > I was planning to carry Paolo's patches, and I can take this one too. Sounds good to me. > If this poses a problem, then I could just do a two-phase commit of > the whitelisting code, leaving the very last commit (which enables the > defense for anything not yet whitelisted), until the KVM trees land. > > What's preferred? Assuming there's an ack from Marc Zyngier on v2 of this patch, I prefer you just take them as part of your series. > > Thanks for looking at this! > No problem, -Christoffer