[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 1/1] virtio crypto device specification: asym
From: |
Zeng, Xin |
Subject: |
Re: [Qemu-devel] [PATCH v1 1/1] virtio crypto device specification: asymmetric crypto service |
Date: |
Sat, 3 Dec 2016 15:23:01 +0000 |
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;
> > +};
> > +
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v1 1/1] virtio crypto device specification: asymmetric crypto service,
Zeng, Xin <=