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. + 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? >
