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

[Venu Busireddy]

On 2020-04-02 15:04:54 -0500, Brijesh Singh wrote:
> 
> On 4/2/20 2:43 PM, Venu Busireddy wrote:
> > On 2020-04-02 14:17:26 -0500, Brijesh Singh wrote:
> >> 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.
> > That is good, and expected behavior.
> >
> >> 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.
> > Agreed. But let us assume that the firmware is not touching memory that
> > it is not supposed to.
> >
> >> Am I missing something ?
> > I believe you are agreeing that if the session blob needs 128 bytes and
> > user space sets params.session_len to 16, the firmware does not copy
> > any data to data->session_address, and sets data->session_len to 128.
> >
> > Now, when we return, won't the user space try to access 128 bytes
> > (params.session_len) of data in params.session_uaddr, and crash? Because,
> > instead of returning an error that buffer is not large enough, we return
> > the call successfully!
> 
> 
> Ah, so the main issue is we should not be going to e_free on error. If
> session_len is less than the expected len then FW will return an error.
> In the case of an error we can skip copying the session_data into
> userspace buffer but we still need to pass the session_len and policy
> back to the userspace.

Sure, that is one way to fix the problem.

> 
> +	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
> +
> +	if (ret)
> +		goto e_free;
> +
> 
> If user space gets an error then it can decode it further to get
> additional information (e.g buffer too small).
> 
> >
> > That is why I was suggesting the following, which you seem to have
> > missed.
> >
> >>> 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?
> > Doesn't the above approach address all scenarios?
> >