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
