[v2,6/7] nbd-client: Stricter enforcing of structured reply spec

Ensure that the server is not sending unexpected chunk lengths
for either the NONE or the OFFSET_DATA chunk, nor unexpected
hole length for OFFSET_HOLE.  This will flag any server that
responds to a zero-length read with an OFFSET_DATA as broken,
even though we previously fixed our client to never be able to
send such a request over the wire.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

09.11.2017 00:57, Eric Blake wrote:
> Ensure that the server is not sending unexpected chunk lengths
> for either the NONE or the OFFSET_DATA chunk, nor unexpected
> hole length for OFFSET_HOLE.  This will flag any server that
> responds to a zero-length read with an OFFSET_DATA as broken,

or OFFSET_HOLE

> even though we previously fixed our client to never be able to
> send such a request over the wire.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   block/nbd-client.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 0a675d0fab..73c9fe0905 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -216,7 +216,7 @@ static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
>       offset = payload_advance64(&payload);
>       hole_size = payload_advance32(&payload);
>
> -    if (offset < orig_offset || hole_size > qiov->size ||
> +    if (!hole_size || offset < orig_offset || hole_size > qiov->size ||
>           offset > orig_offset + qiov->size - hole_size) {
>           error_setg(errp, "Protocol error: server sent chunk exceeding requested"
>                            " region");
> @@ -281,7 +281,8 @@ static int nbd_co_receive_offset_data_payload(NBDClientSession *s,
>
>       assert(nbd_reply_is_structured(&s->reply));
>
> -    if (chunk->length < sizeof(offset)) {
> +    /* The NBD spec requires at least one byte of payload */
> +    if (chunk->length <= sizeof(offset)) {
>           error_setg(errp, "Protocol error: invalid payload for "
>                            "NBD_REPLY_TYPE_OFFSET_DATA");
>           return -EINVAL;
> @@ -293,7 +294,7 @@ static int nbd_co_receive_offset_data_payload(NBDClientSession *s,
>       be64_to_cpus(&offset);
>
>       data_size = chunk->length - sizeof(offset);
> -    if (offset < orig_offset || data_size > qiov->size ||
> +    if (!data_size || offset < orig_offset || data_size > qiov->size ||

!data_size - always false here (because of previous check), and even if it could be zero it
isn't correspond to error message.

without this, or with an assert instead:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


>           offset > orig_offset + qiov->size - data_size) {
>           error_setg(errp, "Protocol error: server sent chunk exceeding requested"
>                            " region");
> @@ -411,6 +412,11 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
>                          " NBD_REPLY_FLAG_DONE flag set");
>               return -EINVAL;
>           }
> +        if (chunk->length) {
> +            error_setg(errp, "Protocol error: NBD_REPLY_TYPE_NONE chunk with"
> +                       " nonzero length");
> +            return -EINVAL;
> +        }
>           return 0;
>       }
>

On 11/09/2017 03:37 AM, Vladimir Sementsov-Ogievskiy wrote:
> 09.11.2017 00:57, Eric Blake wrote:
>> Ensure that the server is not sending unexpected chunk lengths
>> for either the NONE or the OFFSET_DATA chunk, nor unexpected
>> hole length for OFFSET_HOLE.  This will flag any server that
>> responds to a zero-length read with an OFFSET_DATA as broken,
> 
> or OFFSET_HOLE

True, even though our implementation was not doing that.  I can tweak
the commit message.

> 
>> even though we previously fixed our client to never be able to
>> send such a request over the wire.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block/nbd-client.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>

>> @@ -281,7 +281,8 @@ static int
>> nbd_co_receive_offset_data_payload(NBDClientSession *s,
>>
>>       assert(nbd_reply_is_structured(&s->reply));
>>
>> -    if (chunk->length < sizeof(offset)) {
>> +    /* The NBD spec requires at least one byte of payload */
>> +    if (chunk->length <= sizeof(offset)) {
>>           error_setg(errp, "Protocol error: invalid payload for "
>>                            "NBD_REPLY_TYPE_OFFSET_DATA");
>>           return -EINVAL;
>> @@ -293,7 +294,7 @@ static int
>> nbd_co_receive_offset_data_payload(NBDClientSession *s,
>>       be64_to_cpus(&offset);
>>
>>       data_size = chunk->length - sizeof(offset);
>> -    if (offset < orig_offset || data_size > qiov->size ||
>> +    if (!data_size || offset < orig_offset || data_size > qiov->size ||
> 
> !data_size - always false here (because of previous check), and even if
> it could be zero it
> isn't correspond to error message.
> 
> without this, or with an assert instead:

I like the assert instead.

> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 

Thanks for the reviews; I'll queue this series onto my NBD tree with
your suggestions incorporated, and send a pull request for 2.11 shortly.