Re: [PATCH v6 01/14] KVM: SVM: Add KVM_SEV SEND_START command

[Brijesh Singh <brijesh.singh@amd.com>]

On 4/2/20 1:57 PM, Venu Busireddy wrote:
[snip]...

>> The question is, how does a userspace know the session length ? One
>> method is you can precalculate a value based on your firmware version
>> and have userspace pass that, or another approach is set
>> params.session_len = 0 and query it from the FW. The FW spec allow to
>> query the length, please see the spec. In the qemu patches I choose
>> second approach. This is because session blob can change from one FW
>> version to another and I tried to avoid calculating or hardcoding the
>> length for a one version of the FW. You can certainly choose the first
>> method. We want to ensure that kernel interface works on the both cases.
> I like the fact that you have already implemented the functionality to
> facilitate the user space to obtain the session length from the firmware
> (by setting params.session_len to 0). However, I am trying to address
> the case where the user space sets the params.session_len to a size
> smaller than the size needed.
>
> Let me put it differently. Let us say that the session blob needs 128
> bytes, but the user space sets params.session_len to 16. That results
> in us allocating a buffer of 16 bytes, and set data->session_len to 16.
>
> What does the firmware do now?
>
> Does it copy 128 bytes into data->session_address, or, does it copy
> 16 bytes?
>
> If it copies 128 bytes, we most certainly will end up with a kernel crash.
>
> If it copies 16 bytes, then what does it set in data->session_len? 16,
> or 128? If 16, everything is good. If 128, we end up causing memory
> access violation for the user space.

My interpretation of the spec is, if user provided length is smaller
than the FW expected length then FW will reports an error with
data->session_len set to the expected length. In other words, it should
*not* copy anything into the session buffer in the event of failure. If
FW is touching memory beyond what is specified in the session_len then
its FW bug and we can't do much from kernel. Am I missing something ?


>
> Perhaps, this can be dealt a little differently? Why not always call
> sev_issue_cmd(kvm, SEV_CMD_SEND_START, ...) with zeroed out data? Then,
> if the user space has set params.session_len to 0, we return with the
> needed params.session_len. Otherwise, we check if params.session_len is
> large enough, and if not, we return -EINVAL?