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

[Halil Pasic]

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.
> 
>> 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.


>>
>>> +    /* 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.

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.

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