Messages in this thread | | | Subject | Re: [RFC Part2 PATCH v3 15/26] KVM: SVM: Add support for SEV LAUNCH_START command | From | Brijesh Singh <> | Date | Wed, 13 Sep 2017 13:23:08 -0500 |
| |
On 09/13/2017 12:25 PM, Borislav Petkov wrote: ...
>> +static void sev_deactivate_handle(struct kvm *kvm, int *error); >> +static void sev_decommission_handle(struct kvm *kvm, int *error); > > Please move code in a way that you don't need those forward > declarations. Also, I'm wondering if having all the SEV-related code > could live in sev.c or so - svm.c is humongous. >
Yes, svm.c is humongous.
...
>> + >> +static void sev_decommission_handle(struct kvm *kvm, int *error) >> +{ >> + struct sev_data_decommission *data; >> + >> + data = kzalloc(sizeof(*data), GFP_KERNEL); > > Also, better on stack. Please do that for the other functions below too.
Yes, some structures are small and I don't expect them to grow in newer API spec. We should be able to move them on the stack. I will audit the code and make the necessary changes.
....
>> + ret = -EFAULT; >> + if (copy_from_user(¶ms, (void *)argp->data, >> + sizeof(struct kvm_sev_launch_start))) > > Sanity-check params. This is especially important if later we start > using reserved fields. >
Yes, I will add some upper bound check on the length field and add the sanity-check just after copying the parameters from userspace
...
>> + goto e_free; >> + >> + ret = -ENOMEM; >> + start = kzalloc(sizeof(*start), GFP_KERNEL); >> + if (!start) >> + goto e_free; >> + >> + /* Bit 15:6 reserved, must be 0 */ >> + start->policy = params.policy & ~0xffc0; >> + >> + if (params.dh_cert_length && params.dh_cert_address) { > > Yeah, we talked about this already: sanity-checking needed. But you get > the idea. >
Will do
...
> > if (copy_from_user(session_addr, > (void *)params.session_address, > params.session_length)) > > reads better to me. Better yet if you shorten those member names into > s_addr and s_len and so on... > >
Will use your recommendation.
thanks
| |