Re: [Qemu-devel] [PATCH v18 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

On 05/16/2017 05:48 AM, Gonglei (Arei) wrote:

[..]
>>> +\begin{itemize*}
>>> +\item The device MUST set the \field{session_id} field to a unique session
>> identifier when the device finishes processing session creation.
>>
>> I guess only if successfull (that is status == VIRTIO_CRYPTO_OK).
>>
> Sure. Let me add the condition.
> 
>>> +\item The device MUST set the \field{status} field to one of the values of
>> enum VIRTIO_CRYPTO_STATUS.
>>
>> Maybe put this one first. The formulation could be more fortunate in
>> a sense that it's required to set the right status (_OK if successs,
>> _NOTSUPP if ...).
>>
>> What shall happen if the operation fails, e.g. becasue of out of resources?
>>
> That means the driver can't create session, and the following crypto
> operation are forbidden.
> 

I wanted to ask which status code has to be used in what case? Is it always
_ERR or _NOTSUPP?

>> Is there some sort of limit for the amount of live sessions (related to
>> the previous question)? Obviously the device needs storage for the
>> session data. Can the guest DOS attack the host by creating an absurd number
>> of sessions?
>>
> Good question. It's true that the guest can trigger DoS attack.
> What's your opinion about the limit in the spec? Add a new feature?
> 

Adding a new feature does not seem like a good idea to me, because
if the guest can turn of the safety mechanism (not negotiate the
corresponding feature), then we are almost back to where we started.

My idea would be to specify how is the device (and maybe also the driver,
I can't tell) supposed to fail if max_session is reached. The device should
just set some proper status for the request I guess. We could also
make the maximal number of sessions an implementation defined property
of the device, and expose it via configuration space.

Hard limit in the spec is also viable, but in that case we also need
to specify what happens if the limit is reached.

[..]

>>> +\subsubsection{Data Virtqueue}\label{sec:Device Types / Crypto Device /
>> Device Operation / Data Virtqueue}
>>> +
>>> +The driver uses the data virtqueue to transmit crypto operation requests to
s/virtqueue/virtqueues
We may have more than one dataq

>> the device,
>>> +and completes the crypto operations.
>>> +
>>> +Session mode dataq requests are as follows:
>>> +
>>> +\begin{lstlisting}
>>> +struct virtio_crypto_op_data_req {
>>> +    struct virtio_crypto_op_header header;
>>> +
>>> +    union {
>>> +        struct virtio_crypto_sym_data_req   sym_req;
>>> +        struct virtio_crypto_hash_data_req  hash_req;
>>> +        struct virtio_crypto_mac_data_req   mac_req;
>>> +        struct virtio_crypto_aead_data_req  aead_req;
>>> +    } u;
>>> +};
>>> +\end{lstlisting}
>>> +
>>> +Dataq requests for both session and stateless modes are as follows:
>>
>> This ain't very nice, I mean the 'both session and stateless modes' together
> 
> So what's your idea?
> 

As I have pointed out while reviewing the QEMU implementation of the stateless
mode I'm not satisfied with the description of the request format(s).

This here is rather a  cosmetic issue, and my idea was to replace
'Session mode dataq requests' with 'If MUX_MODE feature bit is not
negotiated the dataq requests'. And also replace 'Dataq requests for both
session and stateless modes are' with 'If MUX_MODE feature bit is
negotiated the dataq requests are'.

In my eyes, how these requests are described is a much bigger problem.

>> with the above 'session mode dataq requests are'. In the meanwhile I know
>> what
>> you mean: if the SESSION_MODE feature bit was negotiated.
>>
> Yes, that's what I want. If the MUX_MODE feature bit is negotiated, the
> driver MUST use struct virtio_crypto_op_data_req_mux for requests.
> 
> 
[..]
>>
>> OK, I have read this to the end and I don't think there is anything which 
>> requires
>> a comment at this stage. I would like to concentrate on your QEMU patches
>> first,
>> and I think we have enough questions/issues already.
>>
> 
> Great thanks for your comments. Pls go ahead. :)

Your are welcome.

Regards,
Halil

> 
> Thanks,
> -Gonglei
> 
>