[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [virtio-dev] Re: [v21 1/2] virtio-crypto: Add virtio cr
From: |
Gonglei (Arei) |
Subject: |
Re: [Qemu-devel] [virtio-dev] Re: [v21 1/2] virtio-crypto: Add virtio crypto device specification |
Date: |
Mon, 13 Nov 2017 07:17:32 +0000 |
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