Re: [PATCH v7 44/45] virt: sevguest: Add support to derive key

[Brijesh Singh]

On 11/18/21 10:43 AM, Peter Gonda wrote:
...
> > +       u8 buf[89];
>
> Could we document this magic number?

Yes, I will document from where this number came.

> > +
> > +       if (!arg->req_data || !arg->resp_data)
> > +               return -EINVAL;
> > +
> > +       /* Copy the request payload from userspace */
> > +       if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> > +               return -EFAULT;
> > +
> > +       /* Message version must be non-zero */
> > +       if (!req.msg_version)
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * The intermediate response buffer is used while decrypting the
> > +        * response payload. Make sure that it has enough space to cover the
> > +        * authtag.
> > +        */
> > +       resp_len = sizeof(resp.data) + crypto->a_len;
> > +       if (sizeof(buf) < resp_len)
> > +               return -ENOMEM;
> > +
> > +       /* Issue the command to get the attestation report */
> > +       rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, req.msg_version,
> > +                                 SNP_MSG_KEY_REQ, &req.data, sizeof(req.data), buf, resp_len,
> > +                                 &arg->fw_err);
> > +       if (rc)
> > +               goto e_free;
>
> Should we check the first 32 bits of |data| here since that is a
> status field? If we see 16h here we could return -EINVAL, or better to
> let userspace deal with that error handling?

I was trying to avoid looking into a response structure to keep the 
flexibility; e.g if SNP firmware changes a response format then we don't 
need to have any changes in the driver. The userspace should be able to 
deal with it and it can check the "status" or a new field etc.

thanks