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

[Gonglei (Arei)]

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: address@hidden
> For additional commands, e-mail: address@hidden