Messages in this thread | | | Subject | Re: [PATCH v7 43/45] virt: Add SEV-SNP guest driver | From | Brijesh Singh <> | Date | Thu, 18 Nov 2021 11:32:38 -0600 |
| |
On 11/17/21 5:34 PM, Peter Gonda wrote:
>> +The guest ioctl should be issued on a file descriptor of the /dev/sev-guest device. >> +The ioctl accepts struct snp_user_guest_request. The input and output structure is >> +specified through the req_data and resp_data field respectively. If the ioctl fails >> +to execute due to a firmware error, then fw_err code will be set. > > Should way say what it will be set to? Also Sean pointed out on CCP > driver that 0 is strange to set the error to, its a uint so we cannot > do -1 like we did there. What about all FFs? >
Sure, all FF's works, I can document and use it.
>> +static inline u64 __snp_get_msg_seqno(struct snp_guest_dev *snp_dev) >> +{ >> + u64 count; > > I may be overly paranoid here but how about > `lockdep_assert_held(&snp_cmd_mutex);` when writing or reading > directly from this data? >
Sure, I can do it.
...
>> + >> + if (rc) >> + return rc; >> + >> + rc = verify_and_dec_payload(snp_dev, resp_buf, resp_sz); >> + if (rc) { >> + /* >> + * The verify_and_dec_payload() will fail only if the hypervisor is >> + * actively modifiying the message header or corrupting the encrypted payload. > modifiying >> + * This hints that hypervisor is acting in a bad faith. Disable the VMPCK so that >> + * the key cannot be used for any communication. >> + */ > > This looks great, thanks for changes Brijesh. Should we mention in > comment here or at snp_disable_vmpck() the AES-GCM issues with > continuing to use the key? Or will future updaters to this code > understand already? >
Sure, I can add comment about the AES-GCM.
...
>> + >> +/* See SNP spec SNP_GUEST_REQUEST section for the structure */ >> +enum msg_type { >> + SNP_MSG_TYPE_INVALID = 0, >> + SNP_MSG_CPUID_REQ, >> + SNP_MSG_CPUID_RSP, >> + SNP_MSG_KEY_REQ, >> + SNP_MSG_KEY_RSP, >> + SNP_MSG_REPORT_REQ, >> + SNP_MSG_REPORT_RSP, >> + SNP_MSG_EXPORT_REQ, >> + SNP_MSG_EXPORT_RSP, >> + SNP_MSG_IMPORT_REQ, >> + SNP_MSG_IMPORT_RSP, >> + SNP_MSG_ABSORB_REQ, >> + SNP_MSG_ABSORB_RSP, >> + SNP_MSG_VMRK_REQ, >> + SNP_MSG_VMRK_RSP, > > Did you want to include MSG_ABSORB_NOMA_REQ and MSG_ABSORB_NOMA_RESP here? >
Yes, I can includes those for the completeness.
...
>> +struct snp_report_req { >> + /* message version number (must be non-zero) */ >> + __u8 msg_version; >> + >> + /* user data that should be included in the report */ >> + __u8 user_data[64]; > > Are we missing the 'vmpl' field here? Does those default all requests > to be signed with VMPL0? Users might want to change that, they could > be using a paravisor. >
Good question, so far I was thinking that guest kernel will provide its vmpl level instead of accepted the vmpl level from the userspace. Do you see a need for a userspace to provide this information ?
thanks
| |