| Message ID | 20170724200303.12197-20-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@amd.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.
On 9/14/17 5:35 AM, Borislav Petkov wrote: ... > + >> + 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... This is wrong -- since all the parameters in GET_STATUS is "OUT" hence we don't need to perform copy_from_user. I will fix it. thanks > >> + 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? SEV firmware supports key-sharing, if guest policy allows sharing the key between VMs then we need the firmware->handle. If key-sharing feature is used then firmware->handle of the 1st VM will be passed into the LAUNCH_START of 2nd VM. I still have not coded up anything in qemu for key-sharing and also I am using GET_STATUS command in qemu yet. But wanted to make sure that if we decide to add "info sev-status" command in qemu-monitor to retrieve the SEV state information and then all the information is available to us.
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))) + 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; + + if (copy_to_user((void *) argp->data, ¶ms, + sizeof(struct kvm_sev_guest_status))) + ret = -EFAULT; +e_free: + kfree(data); + return ret; +} + static int svm_memory_encryption_op(struct kvm *kvm, void __user *argp) { struct kvm_sev_cmd sev_cmd; @@ -6055,6 +6089,10 @@ static int svm_memory_encryption_op(struct kvm *kvm, void __user *argp) r = sev_launch_finish(kvm, &sev_cmd); break; } + case KVM_SEV_GUEST_STATUS: { + r = sev_guest_status(kvm, &sev_cmd); + break; + } default: break; }
The command is used for querying the SEV guest status. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/kvm/svm.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)