[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [virtio-dev] Re: [Qemu-devel] [v21 1/2] virtio-crypto: Add virtio crypto device specification
Hi, > > > +The controlq request is composed of two parts: > > +\begin{lstlisting} > > +struct virtio_crypto_op_ctrl_req { > > + struct virtio_crypto_ctrl_header header; > > + > > + /* additional paramenter */ > > + u8 additional_para[addl_para_len]; > > What does additional paramenter mean? Even if I s/paramenter/parameter > id doesn't sit well. To me and in this context additional is kind > of like optional: because each member of a struct is trivially additional > in respect to the previous members, and there is no point in pointing > out additional. I would much rather go with something like: > u8 op_specific[] > > I also don't find the addl_para_len used anywhere. Then IMHO we don't > need to introduce a name. > I'd like to say that the additional_para[addl_para_len] is just a placeholder, which is explained by the below content. I'm fine with op_specific[] too. > > +}; > > +\end{lstlisting} > > + > > +The first is a general header (see above). And the second one, additional > > +paramenter, contains an crypto-service-specific structure, which could be > one > > s/paramenter/parameter > > It's actually opcode specific, or? Or is there a destroy service? > We can choose the specific request (structure) as the *op_specific* according to opcode. > > +of the following types: > > +\begin{itemize*} > > +\item struct virtio_crypto_sym_create_session_req > > +\item struct virtio_crypto_hash_create_session_req > > +\item struct virtio_crypto_mac_create_session_req > > +\item struct virtio_crypto_aead_create_session_req > > +\item virtio_crypto_destroy_session_req > > +\end{itemize*} > > + > > +The size of the additional paramenter depends on the > VIRTIO_CRYPTO_F_MUX_MODE > > s/paramenter/parameter > > > +feature bit: > > +\item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is NOT negotiated, > the > > + size of additional paramenter is fixed to 56 bytes, the data of the > unused > > s/paramenter/parameter > > > + part (if has) will be ingored. > > s/ingored/ignored > > > > +\item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated, the > size of > > + additional paramenter is flexible, which is the same as the > crypto-service-specific > > s/paramenter/parameter > > > + structure used. > > + > > +\paragraph{Session operation}\label{sec:Device Types / Crypto Device / > Device Operation / Control Virtqueue / Session operation} > > + > > +The session is a handle which describes the cryptographic parameters to be > > +applied to a number of buffers. > > + > > +The following structure stores the result of session creation set by the > device: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_session_input { > > + /* Device-writable part */ > > + le64 session_id; > > + le32 status; > > + le32 padding; > > +}; > > +\end{lstlisting} > > + > > +A request to destroy a session includes the following information: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_destroy_session_req { > > + /* Device-readable part */ > > + le64 session_id; > > + /* Device-writable part */ > > + le32 status; > > + le32 padding; > > +}; > > +\end{lstlisting} > > + > > +\subparagraph{Session operation: HASH session}\label{sec:Device Types / > Crypto Device / Device > > +Operation / Control Virtqueue / Session operation / Session operation: HASH > session} > > + > > Let me skip to the one actually implemented. > > > + > > +The request of symmetric session includes two parts, CIPHER algorithms > > +and chain algorithms (chaining CIPHER and HASH/MAC). > > This sounds like concatenation and not either-or. > > + > > +CIPHER session requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_cipher_session_para { > > + /* See VIRTIO_CRYPTO_CIPHER* above */ > > + le32 algo; > > + /* length of key */ > > + le32 keylen; > > +#define VIRTIO_CRYPTO_OP_ENCRYPT 1 > > +#define VIRTIO_CRYPTO_OP_DECRYPT 2 > > + /* encryption or decryption */ > > + le32 op; > > + le32 padding; > > +}; > > + > > +struct virtio_crypto_cipher_session_req { > > + /* Device-readable part */ > > + struct virtio_crypto_cipher_session_para para; > > + /* The cipher key */ > > + u8 cipher_key[keylen]; > > + > > Is there a limit to the size of chiper_key. I don't see one in your > kernel code. OTOH given that virtio_crypto_sym_create_session_req > is one flavor of virtio_crypto_op_ctrl_req.additional_para and that > the later is 56 bytes in case no mux mode is supported, I think > there must be a limit to the size of cipher_key! > Of course the size of cipher_key is limited, firstly the max length is defined in virtio crypto's configuration, see struct virtio_crypto_config { ... ... /* Maximum length of cipher key */ uint32_t max_cipher_key_len; ... ... }; Secondly the real cipher_key size for a specific request, is in struct virtio_crypto_cipher_session_para, struct virtio_crypto_cipher_session_para { ... ... /* length of key */ le32 keylen; ... ... }; That means a size of cipher_key is variable, which is assigned in each request. > Please explain! > > Looking at the kernel code again, it seems to me that chiper_key > starts at offset 72 == sizeof(struct virtio_crypto_op_ctrl_req) > where struct virtio_crypto_op_ctrl_req is defined in > include/uapi/linux/virtio_crypto.h. That would mean that this > guy is *not a part of* virtio_crypto_op_ctrl_req but comes > after it and is of variable size. > > > + /* Device-writable part */ > > > Now I'm interested on what 'offset' does the device writable > part start. > > Of course technically we don't need to know this, because we > have a device-read-only or device-write-only indication on each > descriptor. So virtio_crypto_session_input starts with the first > device write only descriptor. > You are right. We definitely don't need to know this. Pls see Qemu code: virtio_crypto_handle_request(): /* We always touch the last byte, so just see how big in_iov is. */ request->in_len = iov_size(in_iov, in_num); request->in = (void *)in_iov[in_num - 1].iov_base + in_iov[in_num - 1].iov_len - sizeof(struct virtio_crypto_inhdr); iov_discard_back(in_iov, &in_num, sizeof(struct virtio_crypto_inhdr)); The above in_iov means device-writable iov. > > + struct virtio_crypto_session_input input; > > +}; > > +\end{lstlisting} > > + > > +Algorithm chaining requests are as follows: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_alg_chain_session_para { > > +#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER > 1 > > +#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH > 2 [skip] > > +The driver can set the \field{op_type} field in struct > virtio_crypto_sym_create_session_req > > +as follows: VIRTIO_CRYPTO_SYM_OP_NONE: no operation; > VIRTIO_CRYPTO_SYM_OP_CIPHER: Cipher only > > +operation on the data; VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING: > Chain any cipher with any hash > > +or mac operation. > > + > > Based on the stuff written here, it ain't obvious to me at which offset does > the device writable part (that is virtio_crypto_session_input) start. > Why do we need to know the offset? IMO we only need to follow the spec arrangement to fill the specific request packet will not be a problem, such as general header + out_data + in_data. Driver first complete read-only part, then the writable part. The device then parses the request packet this way. Why do we need to write this offset in the spec? > From your kernel code, it seems to me that it starts at offset 72 + keylen. > > > To sum it up I'm awfully dissatisfied. Maybe I made some mistake somewhere, > and that's why things don't make sense. > > I would really appreciate somebody else having a look, and telling: > is it possible to figure out the message formats and create an inter-operable > implementation based on this text (and without looking at the Linux/QEMU > code)? > Yes, we need some other comments about this. Thanks, -Gonglei > > > +\subparagraph{Session operation: AEAD session}\label{sec:Device Types / > Crypto Device / Device > > +Operation / Control Virtqueue / Session operation / Session operation: AEAD > session} > > Further review does not make sense at the moment. If it's just my train of > thought > that got derailed, please put it back on the rails first. > > Regards, > Halil > > [..] > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]