Linux Virtualization - Sep 2021 - [bug report] vdpa_sim_blk: implement ramdisk behaviour

On Wed, Sep 29, 2021 at 02:37:42PM +0300, Dan Carpenter
wrote:
>     89         /* The last byte is the status and we checked if the last
iov has
>     90          * enough room for it.
>     91          */
>     92         to_push = vringh_kiov_length(&vq->in_iov) - 1;
> 
> Are you positive that vringh_kiov_length() cannot be zero?  I looked at
> the range_check() and there is no check for "if (*len == 0)".
> 
>     93 
>     94         to_pull = vringh_kiov_length(&vq->out_iov);
>     95 
>     96         bytes = vringh_iov_pull_iotlb(&vq->vring,
&vq->out_iov, &hdr,
>     97                                       sizeof(hdr));
>     98         if (bytes != sizeof(hdr)) {
>     99                 dev_err(&vdpasim->vdpa.dev, "request out
header too short\n");
>     100                 return false;
>     101         }
>     102 
>     103         to_pull -= bytes;
> 
> The same "bytes" is used for both to_pull and to_push.  In this
> assignment it would only be used for the default case which prints an
> error message.
> 
Sorry, no.  This part is wrong.  "bytes" is not used for
"to_push"
either here or below.  But I still am not sure "*len == 0" or how we
know that "to_push >= VIRTIO_BLK_ID_BYTES".

regards,
dan carpenter

On Wed, Sep 29, 2021 at 02:46:52PM +0300, Dan Carpenter
wrote:
>On Wed, Sep 29, 2021 at 02:37:42PM +0300, Dan Carpenter wrote:
>>     89         /* The last byte is the status and we checked if the
last iov has
>>     90          * enough room for it.
>>     91          */
>>     92         to_push = vringh_kiov_length(&vq->in_iov) - 1;
>>
>> Are you positive that vringh_kiov_length() cannot be zero?  I looked at
>> the range_check() and there is no check for "if (*len == 0)".
>>
>>     93
>>     94         to_pull = vringh_kiov_length(&vq->out_iov);
>>     95
>>     96         bytes = vringh_iov_pull_iotlb(&vq->vring,
&vq->out_iov, &hdr,
>>     97                                       sizeof(hdr));
>>     98         if (bytes != sizeof(hdr)) {
>>     99                 dev_err(&vdpasim->vdpa.dev, "request
out header too short\n");
>>     100                 return false;
>>     101         }
>>     102
>>     103         to_pull -= bytes;
>>
>> The same "bytes" is used for both to_pull and to_push.  In
this
>> assignment it would only be used for the default case which prints an
>> error message.
>>
>
>Sorry, no.  This part is wrong.  "bytes" is not used for
"to_push"
>either here or below.  But I still am not sure "*len == 0" or how
we
At line 84 we check that the last `in_iov` has at least one byte, so 
vringh_kiov_length(&vq->in_iov) cannot be zero.
It will return the sum of all lengths, so at least 1.

Maybe better to add a comment.
>know that "to_push >= VIRTIO_BLK_ID_BYTES".
vringh_iov_push_iotlb() will push at least the bytes available in 
`in_iov`, if these are less, it will copy less bytes of 
VIRTIO_BLK_ID_BYTES.

Maybe here it would be better to add a check because the driver isn't 
following the specification.

And I'd avoid the subtraction highlighted by Smatch static checker.

Thanks for reporting. I'll send patches to fix these issues.

Stefano