lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170914103539.6q3clh3qcynkwgeo@pd.tnic> Date: Thu, 14 Sep 2017 12:35:39 +0200 From: Borislav Petkov <bp@...e.de> To: Brijesh Singh <brijesh.singh@....com> Cc: linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>, Joerg Roedel <joro@...tes.org>, "Michael S . Tsirkin" <mst@...hat.com>, Paolo Bonzini <pbonzini@...hat.com>, \"Radim Krčmář\" <rkrcmar@...hat.com>, Tom Lendacky <thomas.lendacky@....com> Subject: Re: [RFC Part2 PATCH v3 19/26] KVM: svm: Add support for SEV GUEST_STATUS command On Mon, Jul 24, 2017 at 03:02:56PM -0500, Brijesh Singh wrote: > The command is used for querying the SEV guest status. > > Signed-off-by: Brijesh Singh <brijesh.singh@....com> > --- > arch/x86/kvm/svm.c | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 7a77197..21f85e1 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -6024,6 +6024,40 @@ static int sev_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp) > return ret; > } > > +static int sev_guest_status(struct kvm *kvm, struct kvm_sev_cmd *argp) > +{ > + struct kvm_sev_guest_status params; > + struct sev_data_guest_status *data; > + int ret; > + > + if (!sev_guest(kvm)) > + return -ENOTTY; > + > + if (copy_from_user(¶ms, (void *) argp->data, > + sizeof(struct kvm_sev_guest_status))) Let me try to understand what's going on here. You copy user data into params... > + return -EFAULT; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->handle = sev_get_handle(kvm); > + ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, data, &argp->error); > + if (ret) > + goto e_free; > + > + params.policy = data->policy; > + params.state = data->state; > + params.handle = data->handle; ... *overwrite* the copied data which means, the copy meant *absolutely* *nothing* at all! ... Also, why does userspace need to know the firmware ->handle? > + > + if (copy_to_user((void *) argp->data, ¶ms, > + sizeof(struct kvm_sev_guest_status))) ... and here you copy it back. And the caller svm_memory_encryption_op() copies sev_cmd yet again! Probably for the sev_cmd.error value. Ok, looking at other commands, they use more members like fd in sev_guest_init(), for example. But please audit all that shuffling of data back and forth and bring it down to a mininum. No useless work pls. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
Powered by blists - more mailing lists