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