Messages in this thread | | | Subject | Re: [PATCH Part1 v5 37/38] virt: sevguest: Add support to derive key | From | Brijesh Singh <> | Date | Wed, 8 Sep 2021 16:44:37 -0500 |
| |
On 9/8/21 9:00 AM, Borislav Petkov wrote: > On Fri, Aug 20, 2021 at 10:19:32AM -0500, Brijesh Singh wrote: >> +2.2 SNP_GET_DERIVED_KEY >> +----------------------- >> +:Technology: sev-snp >> +:Type: guest ioctl >> +:Parameters (in): struct snp_derived_key_req >> +:Returns (out): struct snp_derived_key_req on success, -negative on error >> + >> +The SNP_GET_DERIVED_KEY ioctl can be used to get a key derive from a root key. >> +The derived key can be used by the guest for any purpose, such as sealing keys >> +or communicating with external entities. >> + >> +The ioctl uses the SNP_GUEST_REQUEST (MSG_KEY_REQ) command provided by the >> +SEV-SNP firmware to derive the key. See SEV-SNP specification for further details >> +on the various fileds passed in the key derivation request. >> + >> +On success, the snp_derived_key_resp.data will contains the derived key > > "will contain"
Noted.
>> + >> + /* Copy the request payload from the userspace */ > > "from userspace"
Noted.
>> + >> static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) >> { >> struct snp_guest_dev *snp_dev = to_snp_dev(file); >> @@ -320,6 +364,10 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long >> ret = get_report(snp_dev, &input); >> break; >> } >> + case SNP_GET_DERIVED_KEY: { >> + ret = get_derived_key(snp_dev, &input); >> + break; >> + } > > {} brackets are not needed. > > What, however, is bothering me more in this function is that you call > the respective ioctl function which might fail, you do not look at the > return value and copy_to_user() unconditionally. > > Looking at get_derived_key(), for example, if it returns after: > > if (!arg->req_data || !arg->resp_data) > return -EINVAL; > > you will be copying the same thing back to the user, you copied in > earlier. That doesn't make any sense to me.
I will look into improving it to copy back to userspace only if there is firmware error.
thanks
| |