Re: [Qemu-devel] [virtio-dev] Re: [v21 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

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