Re: [Qemu-devel] [PATCH v1 1/1] virtio crypto device specification: asymmetric crypto service

[Zeng, Xin]

Hi, Lei:
   Thanks for the comments. 

On Wednesday, November 30, 2016 9:25 AM, Gonglei (Arei) wrote:
> >
> >
> >  \subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
> > @@ -44,7 +44,9 @@ struct virtio_crypto_config {
> >      le32 mac_algo_l;
> >      le32 mac_algo_h;
> >      le32 aead_algo;
> > -    le32 reserve;
> > +    le32 asym_algo;
> > +    le32 rsa_padding;
> > +    le32 reserved;
> >  };
> 
> Please rebased on the latest virtio crypto spec,
> the config structure had been changed.
> 

Sure.

> >  \end{lstlisting}
> >
> > @@ -70,6 +72,8 @@ The following services are defined:
> >  #define VIRTIO_CRYPTO_SERVICE_MAC    (2)
> >  /* AEAD (Authenticated Encryption with Associated Data) service */
> >  #define VIRTIO_CRYPTO_SERVICE_AEAD   (3)
> > +/* ASYM (Asymmetric crypto algorithms) service */
> > +#define VIRTIO_CRYPTO_SERVICE_ASYM   (4)
> >  \end{lstlisting}
> >
> >  The last driver-read-only fields specify detailed algorithms masks
> > @@ -143,6 +147,28 @@ The following AEAD algorithms are defined:
> >  #define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305  3
> >  \end{lstlisting}
> >
> > +The following asymmetric algorithms are defined:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_ASYM_NONE        0
> > +#define VIRTIO_CRYPTO_ASYM_RSA         1
> > +#define VIRTIO_CRYPTO_ASYM_DSA         2
> > +#define VIRTIO_CRYPTO_ASYM_DH          3
> > +#define VIRTIO_CRYPTO_ASYM_ECDSA       4
> > +#define VIRTIO_CRYPTO_ASYM_ECDH           5
> 
> Mixing tab and space.
> 

Will fix it in next version.

> > +\end{lstlisting}
> > +
> > +The following rsa padding capabilities are defined:
> > +
> 
> What're the padding capabilities used for?
> I think we should add some explanation for them.
> 

Makes sense, let's explain it more.

> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_RSA_NO_PADDING         0
> > +#define VIRTIO_CRYPTO_RSA_PKCS1_PADDING      1
> > +#define VIRTIO_CRYPTO_RSA_SSLV23_PADDING     2
> > +#define VIRTIO_CRYPTO_RSA_PKCS1_OAEP_PADDING 3
> > +#define VIRTIO_CRYPTO_RSA_X931_PADDING       4
> > +#define VIRTIO_CRYPTO_RSA_PKCS1_PSS_PADDING  5
> > +\end{lstlisting}
> > +
> >  \begin{note}
> >  Any other value is reserved for future use.
> >  \end{note}
> > @@ -241,6 +267,18 @@ struct virtio_crypto_op_header {
> >      VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> >  #define VIRTIO_CRYPTO_AEAD_DECRYPT \
> >      VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> > +#define VIRTIO_CRYPTO_ASYM_SIGN    \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x00)
> > +#define VIRTIO_CRYPTO_ASYM_VERIFY \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x01)
> > +#define VIRTIO_CRYPTO_ASYM_ENCRYPT  \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x02)
> > +#define VIRTIO_CRYPTO_ASYM_DECRYPT  \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x03)
> > +#define VIRTIO_CRYPTO_ASYM_KEY_GEN  \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x04)
> > +#define VIRTIO_CRYPTO_ASYM_KEY_EXCHG \
> > +    VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_ASYM, 0x05)
> >      le32 opcode;
> >      /* algo should be service-specific algorithms */
> >      le32 algo;
> > @@ -548,6 +586,23 @@ struct virtio_crypto_op_data_req {
> >          struct virtio_crypto_hash_data_req  hash_req;
> >          struct virtio_crypto_mac_data_req   mac_req;
> >          struct virtio_crypto_aead_data_req  aead_req;
> > +
> > +        struct virtio_crypto_ecdsa_sign_req ecdsa_sign_req;
> > +        struct virtio_crypto_dsa_sign_req dsa_sign_req;
> > +        struct virtio_crypto_rsa_sign_req rsa_sign_req;
> > +        struct virtio_crypto_ecdsa_verify_req ecdsa_verify_req;
> > +        struct virtio_crypto_dsa_verify_req dsa_verify_req;
> > +        struct virtio_crypto_rsa_verify_req rsa_verify_req;
> > +        struct virtio_crypto_rsa_enc_req rsa_enc_req
> > +        struct virtio_crypto_rsa_dec_req rsa_dec_req;
> > +        struct virtio_crypto_rsa_keygen_req rsa_keygen_req;
> > +        struct virtio_crypto_dsa_keygen_req dsa_keygen_req;
> > +        struct virtio_crypto_ec_keygen_req ec_keygen_req;
> > +        struct virtio_crypto_dh_keyexchg_param_gen_req
> > dh_keyexchg_param_gen_req;
> > +        struct virtio_crypto_dh_keyexchg_key_gen_req
> > dh_keyexchg_key_gen_req;
> > +        struct virtio_crypto_dh_keyexchg_key_compute_req
> > dh_keyexchg_key_compute_req;
> > +        struct virtio_crypto_ecdh_keyexchg_key_gen_req
> > ecdh_keyexchg_key_gen_req;
> > +        struct virtio_crypto_ecdh_keyexchg_key_compute_req
> > ecdh_keyexchg_key_compute_req;
> >      } u;
> >  };
> >  \end{lstlisting}
> > @@ -996,4 +1051,908 @@ pointed by \field{dst_data_addr} and
> > \field{digest_result_addr} in struct virtio
> >  \item The device MUST copy the digest result to the guest memory
> recorded
> > by \field{digest_result_addr} field in struct virtio_crypto_aead_input.
> >  \item The device MUST set the \field{status} field in strut
> > virtio_crypto_aead_input: VIRTIO_CRYPTO_OK: success;
> VIRTIO_CRYPTO_ERR:
> > creation failed or device error; VIRTIO_CRYPTO_NOTSUPP: not supported.
> >  \item When the \field{opcode} is VIRTIO_CRYPTO_AEAD_DECRYPT, the
> device
> > MUST verify and return the verification result to the driver, and if the
> > verification result is incorrect, VIRTIO_CRYPTO_BADMSG (bad message)
> MUST
> > be returned to the driver.
> > -\end{itemize*}
> > \ No newline at end of file
> > +\end{itemize*}
> > +
> > +\subsubsection{Asymmetric Crypto Service Operation}\label{sec:Device
> Types
> > / Crypto Device / Device Operation /
> > +Crypto operation / Asymmetric Crypto Service Operation}
> > +
> > +In general, asymmetric crypto service can be referred as signature,
> > verification,
> > +encryption, decryption, key generation and key exchange. Not each
> algorithm
> > supports
> > +all these services.
> > +
> > +Unlike symmetric crypto service, asymmetric crypto service normally
> does't
> > +need session operations, the request is encapsulated within one packet
> which
> > is represented
> > +by \field{virtio_crypto_op_data_req}.
> > +(see \ref{sec:Device Types / Crypto Device / Device Operation / Data
> > Virtqueue}).
> > +
> > +Once service request is handled by the device, the device writes back the
> > operation results to the corresponding
> > +fields in the request packet.
> > +
> > +The asymmetric crypto operation uses a virtio_crypto_buf to represent
> the
> > input/output buffer.
> > +\begin{lstlisting}
> > +struct virtio_crypto_buf {
> > +    le32 len;
> > +    le32 reserved;
> > +    le64 addr;
> > +};
> > +\end{lstlisting}
> > +
> 
> This is the same problem with my pervious symmetric spec.
> Pls don't define a private structure in the virtio spec,
> and refer to the latest virtio crypto spec:
> 
> The buffer length is stored in the para part, and
> the buffer content is stored in input/output parts by u8[len].
> 

Right, will update those.

> struct virtio_crypto_alg_chain_session_para {
>     le32 alg_chain_order;
>     le32 hash_mode;
>     struct virtio_crypto_cipher_session_para cipher_param;
>     union {
>         struct virtio_crypto_hash_session_para hash_param;
>         struct virtio_crypto_mac_session_para mac_param;
>     } u;
>     /* length of the additional authenticated data (AAD) in bytes */
>     le32 aad_len;
>     le32 padding;
> };
> 
> struct virtio_crypto_alg_chain_session_req {
>     /* Device-readable part */
>     struct virtio_crypto_alg_chain_session_para para;
>     /* The cipher key */
>     u8 cipher_key[keylen];
>     /* The authenticated key */
>     u8 auth_key[auth_key_len];
> 
>     /* Device-writable part */
>     struct virtio_crypto_session_input input;
> };
> 
> > +\devicenormative{\paragraph}{Asymmetric Crypto Service
> Operation}{Device
> > Types / Crypto Device / Device Operation / Asymmetric Crypto Service
> > Operation}
> > +\begin{itemize*}
> > +\item The device MUST parse the field \field{opcode} within
> > \field{virtio_crypto_op_header} (see \ref{sec:Device Types / Crypto
> Device /
> > Device Operation}) before
> > +it handles the corresponding service request.
> > +\item The device SHOULD set the operation status in field \field{status}
> within
> > \field{idata}.
> > +\item The device SHOULD update the operation result to the fields in
> > \field{idata} if the operation is successful.
> > +\end{itemize*}
> > +
> > +\drivernormative{\paragraph}{Asymmetric Crypto Service
> Operation}{Device
> > Types / Crypto Device / Device Operation / Asymmetric Crypto Service
> > Operation}
> > +\begin{itemize*}
> > +\item The driver SHOULD set the corresponding \field{opcode} in
> > \field{virtio_crypto_op_header} according to different services.
> > +\item The driver SHOULD offer operatable buffer within \field{idata} if the
> > service request structure defines.
> > +\end{itemize*}
> > +
> > +\paragraph{Signature: ECDSA signature operation}\label{sec:Device Types
> /
> > Crypto Device /
> > +Device Operation / Crypto operation / Asymmetric Crypto Service
> Operation /
> > Signature: ECDSA signature operation}
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_ec_group {
> > +    struct virtio_crypto_buf xg;
> > +    struct virtio_crypto_buf yg;
> > +    struct virtio_crypto_buf n;
> > +    struct virtio_crypto_buf q;
> > +    struct virtio_crypto_buf a;
> > +    struct virtio_crypto_buf b;
> > +    struct virtio_crypto_buf h;
> > +};
> > +
> > +#define VIRTIO_CRYPTO_EC_FIELD_TYPE_PRIME 0
> > +#define VIRTIO_CRYPTO_EC_FIELD_TYPE_BINARY 1
> > +
> > +struct virtio_crypto_ecdsa_sign_para{
> > +    /* Hash alogrithms applied to msg */
> > +    le32 hash_algo;
> > +    /* EC field_type, see VIRTIO_CRYPTO_EC_FIELD_TYPE_* definition */
> > +    le32 field_type;
> > +
> > +    /* EC Group parameters */
> > +    struct virtio_crypto_ec_group ec;
> > +    /* Random value */
> > +    struct virtio_crypto_buf k;
> > +    /* Private Key */
> > +    struct virtio_crypto_buf d;
> > +};
> > +
> > +struct virtio_crypto_ecdsa_sign_output {
> > +    /* Message need to be signed */
> > +    struct virtio_crypto_buf msg;
> > +};
> > +
> > +struct virtio_crypto_ecdsa_sign_input {
> > +    /* operation result */
> > +    le32 status;
> > +    le32 reserved;
> > +
> > +    /* signature result */
> > +    struct virtio_crypto_buf r;
> > +    struct virtio_crypto_buf s;
> > +};
> > +\end{lstlisting}
> > +
> > +ECDSA signature operation request is defined as below:
> > +\begin{lstlisting}
> > +struct virtio_crypto_ecdsa_sign_req {
> > +    struct virtio_crypto_ecdsa_sign_para parameter;
> > +    struct virtio_crypto_ecdsa_sign_output odata;
> > +
> > +    struct virtio_crypto_ecdsa_sign_input idata;
> > +};
> > +\end{lstlisting}
> > +
> > +
> > +\devicenormative{\subparagraph}{ECDSA signature operation}{Device
> Types /
> > Crypto Device / Device Operation / Asymmetric Crypto Service Operation /
> > ECDSA signature operation}
> > +\begin{itemize*}
> > +\item The device SHOULD set the operation results in \field{idata}
> according
> 
> Here you say SHOULD, but...
> 
> > to the general device requirments for asymmetric crypto service.
> > +\end{itemize*}
> > +
> > +\drivernormative{\subparagraph}{ECDSA signature operation}{Device
> Types /
> > Crypto Device / Device Operation / Asymmetric Crypto Service Operation /
> > ECDSA signature operation}
> > +\begin{itemize*}
> > +\item The driver SHOULD set the \field{opcode} in
> > \field{virtio_crypto_op_header} to VIRTIO_CRYPTO_ASYM_SIGN,
> > +set \field{algo} to VIRTIO_CRYPTO_ASYM_ECDSA.
> > +\item The driver SHOULD set all fields in \field{parameter} and
> \field{odata}
> > except field \field{reserved}.
> > +\item The driver MUST check the operation result \field{status} in
> \field{idata}
> > before it operates other fields in \field{idata}.
> 
> ...here is MUST.
> 
> Maybe s/SHOULD/MUST/g, no?
> 

Yes, will fix them in next version.

> > +Only if the operation is successful, the \field{r,s} in \field{idata} are
> operatable.
> > +\end{itemize*}
> > +
> > +\paragraph{Signature: DSA signature operation}\label{sec:Device Types /
> > Crypto Device /
> > +Device Operation / Crypto operation / Asymmetric Crypto Service
> Operation/
> > Signature: DSA signature operation}
> > +
> > +\begin{lstlisting}
> > +struct virtio_crypto_dsa_group {
> > +    struct virtio_crypto_buf p;
> > +    struct virtio_crypto_buf q;
> > +    struct virtio_crypto_buf g;
> > +};
> > +
> 
> Pls don't use struct virtio_crypto_buf.
> 
> It seems all below algorithms have the same style,
> so I stop reviewing here.
> 
> Regards,
> -Gonglei
> 
> > +struct virtio_crypto_dsa_sign_para {
> > +    /* Hash alogrithms applied to msg */
> > +    le32 hash_algo;
> > +    le32 reserved;
> > +
> > +    /* DSA group parameter */
> > +    struct virtio_crypto_dsa_group dsa;
> > +    /* Random value */
> > +    struct virtio_crypto_buf k;
> > +    /* Private Key */
> > +    struct virtio_crypto_buf x;
> > +};
> > +