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