Hello Halil, Thanks for your feedback.
> > On 11/13/2017 08:17 AM, Gonglei (Arei) wrote: > >>> +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; > > ... ... > > }; > > > > So for the current qemu implementation it's 64 bytes. > > > 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. > > Of course I understood that. There are two problems I was trying > to point out, and you ignored both. > > 1. The more important one I was explicit about. Sadly you ignored > that part of my mail. I will mark it as *Problem 1* down below. > > 2. If there is a limit to the size, then there should be a driver > normative statement ("Driver Requirements") that states this limit > MUST be respected. I didn't find this statement. We can add it. > > > >> 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. > > *Problem 1* > > Now consider the part where the whole request is described > > """ > +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]; > +}; > +\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 > +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 > +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 > + part (if has) will be ingored. > +\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 > + structure used. > """ > > There it's said that the whole request is header + additional_para and that > if VIRTIO_CRYPTO_F_MUX_MODE feature bit is NOT negotiated additional > para > is 56 bytes. Let's assume the key is part of the additional parameter. > But you can't put 64 bytes into 56 bytes. So as I say above > *the key is not part of virtio_crypto_op_ctrl_req* neiter as described > in this spec nor as defined in uapi/linux/virtio_crypto.h. That means > the communication protocol description (more precisely the message format > description) in the spec is broken. QED > > In my opinion this is a big issue. > OK, I get your point now. Sorry about that. :( We should update the description about cipher_key and something like that. The key is indeed not a part of virtio_crypto_op_ctrl_req in the realization, is a separate entry in the descriptor table. > > >> > >>> + /* 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. > > > > I've seen that code. Thanks. > > >>> + 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? > > > > We actually don't. That is just what I've said a couple of lines before. > But we do need to know at which offset the key is (or any other piece of > information within the device readable and the device writable part). > > Maybe it was not smart from me to ask this question. > > I understood this specification as if it were trying to describe a message > format as structs, which are basically supposed to define the offset of the > individual fields. So for a guy trying to figure out the spec (because he > wants to implement it) asking himself at what offset a certain piece > of info is, is natural IMHO. > OK, I agree. The cipher key offset is 72. Will add some more description about those parameters. > I don't think the placement of virtio_crypto_session_input is adequately > described. Please point me to the fragment if I've missed it. IMHO a > straightforward way to describe it would be that virtio_crypto_session_input > starts at the data address designated by the first device writable descriptor. > Make sense to me. Thanks! > Regards, > Halil > > >> 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