Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/16/2017 04:52 AM, Gonglei (Arei) wrote:
>>
>> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote:
>>>
>>>> From: Halil Pasic [mailto:address@hidden
>>>> Sent: Friday, May 12, 2017 7:02 PM
>>>>
>>>>
>>>> On 05/08/2017 01:38 PM, Gonglei wrote:
>>>>> According to the new spec, we should use different
>>>>> requst structure to store the data request based
>>>>> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
>>>>> negotiated or not.
>>>>>
>>>>> In this patch, we havn't supported stateless mode
>>>>> yet. The device reportes an error if both
>>>>> VIRTIO_CRYPTO_F_MUX_MODE and
>>>> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
>>>>> are negotiated, meanwhile the header.flag doesn't set
>>>>> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
>>>>>
>>>>> Let's handle this scenario in the following patches.
>>>>>
>>>>> Signed-off-by: Gonglei <address@hidden>
>>>>> ---
>>>>>  hw/virtio/virtio-crypto.c | 83
>>>> ++++++++++++++++++++++++++++++++++++++++-------
>>>>>  1 file changed, 71 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
>>>>> index 0353eb6..c4b8a2c 100644
>>>>> --- a/hw/virtio/virtio-crypto.c
>>>>> +++ b/hw/virtio/virtio-crypto.c
>>>>> @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>> *request)
>>>>>      VirtQueueElement *elem = &request->elem;
>>>>>      int queue_index =
>>>> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
>>>>>      struct virtio_crypto_op_data_req req;
>>>>> +    struct virtio_crypto_op_data_req_mux req_mux;
>>>>>      int ret;
>>>>>      struct iovec *in_iov;
>>>>>      struct iovec *out_iov;
>>>>> @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>> *request)
>>>>>      uint64_t session_id;
>>>>>      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
>>>>>      Error *local_err = NULL;
>>>>> +    bool mux_mode_is_negotiated;
>>>>> +    struct virtio_crypto_op_header *header;
>>>>> +    bool is_stateless_req = false;
>>>>>
>>>>>      if (elem->out_num < 1 || elem->in_num < 1) {
>>>>>          virtio_error(vdev, "virtio-crypto dataq missing headers");
>>>>> @@ -597,12 +601,28 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>> *request)
>>>>>      out_iov = elem->out_sg;
>>>>>      in_num = elem->in_num;
>>>>>      in_iov = elem->in_sg;
>>>>> -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>>> -                != sizeof(req))) {
>>>>> -        virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>>> -        return -1;
>>>>> +
>>>>> +    mux_mode_is_negotiated =
>>>>> +        virtio_vdev_has_feature(vdev,
>> VIRTIO_CRYPTO_F_MUX_MODE);
>>>>> +    if (!mux_mode_is_negotiated) {
>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>>> +                    != sizeof(req))) {
>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>>> +            return -1;
>>>>> +        }
>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req));
>>>>> +
>>>>> +        header = &req.header;
>>>>> +    } else {
>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
>>>>> +                sizeof(req_mux)) != sizeof(req_mux))) {
>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>>> +            return -1;
>>>>> +        }
>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
>>>>> +
>>>>> +        header = &req_mux.header;
>>>>
>>>> I wonder if this request length checking logic is conform to the
>>>> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
>>>> virtio crypto device specification").
>>>>
>>> Sure. Please see below normative formulation:
>>>
>>> '''
>>> \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types
>> / Crypto Device / Device Operation / Symmetric algorithms Operation}
>>> ...
>>> \item If the VIRTIO_CRYPTO_F_MUX_MODE feature bit is negotiated, the
>> driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto
>> requests.
>>> Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
>>> ...
>>> '''
>>>
>>
>> As far as I can remember, we have already agreed that in terms of the
>> spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your
> 
> Sorry, I don't think so. :(
> 
>> code you have a substantially different struct virtio_crypto_op_data_req
>> than in your spec! For instance in the spec virtio_crypto_op_data_req is
>> the full request and contains the data buffers (src_data and the
>> dest_data), while in your code it's effectively just a header and does
>> not contain any data buffers.
>>
> I said struct virtio_crypto_op_data_req in the spec is just a symbol.
> I didn't find a better way to express the src_data and dst_data etc. So
> I used u8[len] xxx_data to occupy a sit in the request.
> 

OK, tell me how is the reader/implementer of the spec supposed to figure
out that a 124 byte padded "header" needs to be precede any "data"?

Besides if you look at

+Stateless mode HASH service requests are as follows:
+
+\begin{lstlisting}
+struct virtio_crypto_hash_para_statelesss {
+    struct {
+        /* See VIRTIO_CRYPTO_HASH_* above */
+        le32 algo;
+    } sess_para;
+
+    /* length of source data */
+    le32 src_data_len;
+    /* hash result length */
+    le32 hash_result_len;
+    le32 reserved;
+};
+struct virtio_crypto_hash_data_req_stateless {
+    /* Device-readable part */
+    struct virtio_crypto_hash_para_stateless para;
+    /* Source data */
+    u8 src_data[src_data_len];
+
+    /* Device-writable part */
+    /* Hash result data */
+    u8 hash_result[hash_result_len];
+    struct virtio_crypto_inhdr inhdr;
+};
+\end{lstlisting}
+

from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device
specification". You need the padding to 128 bytes after
virtio_crypto_hash_para_stateless para, but before src_data. But there is
no indication in the definition of virtio_crypto_hash_data_req_stateless
nor somewhere in the spec about that. On the contrary based on this
one would expect para.reserved and src_data being pretty adjecent.

Thus the normative statement you quoted is (IMHO) ill suited and
insufficient to express what you have been trying to express.

>>
>>>> AFAIU here you allow only requests of two sizes: one fixed size
>>>> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
>>>> means that some requests need quite some padding between what
>>>> you call the 'request' and the actual data on which the crypto
>>>> operation dictated by the 'request' needs to be performed.
>>>
>>> Yes, that's true.
>>>
>>
>> This implies that should we need more than 128 bytes for a request,
>> we will need to introduce jet another request format and negotiate it
>> via feature bits.
>>
> Why do we need other feature bits?

Because assume you have something that needs more that 128 bytes for
its request, how do you solve the problem without new format end new
feature bit?

> 
>> By the way, I'm having a hard time understing how is the requirement form
>> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260
>> 004
>> (2.4.4 Message Framing) satisfied by this code. Could you explain this
>> to me please?
>>
> Sorry for my bad English, 
> I don't know which normative formulation the code violates?

I'm not sure it's actually violated, but I'm concerned about the following
normative statement: "The device MUST NOT make assumptions about the particular
arrangement of descriptors. The device MAY have a reasonable limit of
descriptors it will allow in a chain." 

Please also read the explanatory part I've linked, because the normative
statement is pretty abstract.

In my understanding, the spec says, that e.g. the virti-crypto device
should not fail if a single request is split up into let's say two chunks
and transmitted by the means of two top level descriptors.

Do you agree with my reading of the spec? 

What does the virtio-crypto device do if it encounters such a situation?

Regards,
Halil