[bug report] vdpa_sim_blk: implement ramdisk behaviour

* [bug report] vdpa_sim_blk: implement ramdisk behaviour
@ 2021-09-29 11:37 Dan Carpenter
  2021-09-29 11:46 ` Dan Carpenter
  0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-09-29 11:37 UTC (permalink / raw)
  To: sgarzare; +Cc: virtualization

Hello Stefano Garzarella,

The patch 7d189f617f83: "vdpa_sim_blk: implement ramdisk behaviour"
from Mar 15, 2021, leads to the following
Smatch static checker warning:

	drivers/vdpa/vdpa_sim/vdpa_sim_blk.c:179 vdpasim_blk_handle_req()
	warn: unsigned subtraction: 'to_push - pushed' use '!='

drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
    61 static bool vdpasim_blk_handle_req(struct vdpasim *vdpasim,
    62                                    struct vdpasim_virtqueue *vq)
    63 {
    64         size_t pushed = 0, to_pull, to_push;
    65         struct virtio_blk_outhdr hdr;
    66         ssize_t bytes;
    67         loff_t offset;
    68         u64 sector;
    69         u8 status;
    70         u32 type;
    71         int ret;
    72 
    73         ret = vringh_getdesc_iotlb(&vq->vring, &vq->out_iov, &vq->in_iov,
    74                                    &vq->head, GFP_ATOMIC);
    75         if (ret != 1)
    76                 return false;
    77 
    78         if (vq->out_iov.used < 1 || vq->in_iov.used < 1) {
    79                 dev_err(&vdpasim->vdpa.dev, "missing headers - out_iov: %u in_iov %u\n",
    80                         vq->out_iov.used, vq->in_iov.used);
    81                 return false;
    82         }
    83 
    84         if (vq->in_iov.iov[vq->in_iov.used - 1].iov_len < 1) {
    85                 dev_err(&vdpasim->vdpa.dev, "request in header too short\n");
    86                 return false;
    87         }
    88 
    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.

    104 
    105         type = vdpasim32_to_cpu(vdpasim, hdr.type);
    106         sector = vdpasim64_to_cpu(vdpasim, hdr.sector);
    107         offset = sector << SECTOR_SHIFT;
    108         status = VIRTIO_BLK_S_OK;
    109 
    110         switch (type) {
    111         case VIRTIO_BLK_T_IN:
    112                 if (!vdpasim_blk_check_range(sector, to_push)) {
    113                         dev_err(&vdpasim->vdpa.dev,
    114                                 "reading over the capacity - offset: 0x%llx len: 0x%zx\n",
    115                                 offset, to_push);
    116                         status = VIRTIO_BLK_S_IOERR;
    117                         break;
    118                 }
    119 
    120                 bytes = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov,
    121                                               vdpasim->buffer + offset,
    122                                               to_push);
    123                 if (bytes < 0) {
    124                         dev_err(&vdpasim->vdpa.dev,
    125                                 "vringh_iov_push_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
    126                                 bytes, offset, to_push);
    127                         status = VIRTIO_BLK_S_IOERR;
    128                         break;
    129                 }
    130 
    131                 pushed += bytes;
    132                 break;
    133 
    134         case VIRTIO_BLK_T_OUT:
    135                 if (!vdpasim_blk_check_range(sector, to_pull)) {
    136                         dev_err(&vdpasim->vdpa.dev,
    137                                 "writing over the capacity - offset: 0x%llx len: 0x%zx\n",
    138                                 offset, to_pull);
    139                         status = VIRTIO_BLK_S_IOERR;
    140                         break;
    141                 }
    142 
    143                 bytes = vringh_iov_pull_iotlb(&vq->vring, &vq->out_iov,
    144                                               vdpasim->buffer + offset,
    145                                               to_pull);

Here "bytes" is to_pull again.

    146                 if (bytes < 0) {
    147                         dev_err(&vdpasim->vdpa.dev,
    148                                 "vringh_iov_pull_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",
    149                                 bytes, offset, to_pull);
    150                         status = VIRTIO_BLK_S_IOERR;
    151                         break;
    152                 }
    153                 break;
    154 
    155         case VIRTIO_BLK_T_GET_ID:
    156                 bytes = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov,
    157                                               vdpasim_blk_id,
    158                                               VIRTIO_BLK_ID_BYTES);

Do we know that to_push is larger than VIRTIO_BLK_ID_BYTES?

    159                 if (bytes < 0) {
    160                         dev_err(&vdpasim->vdpa.dev,
    161                                 "vringh_iov_push_iotlb() error: %zd\n", bytes);
    162                         status = VIRTIO_BLK_S_IOERR;
    163                         break;
    164                 }
    165 
    166                 pushed += bytes;
    167                 break;
    168 
    169         default:
    170                 dev_warn(&vdpasim->vdpa.dev,
    171                          "Unsupported request type %d\n", type);
    172                 status = VIRTIO_BLK_S_IOERR;
    173                 break;
    174         }
    175 
    176         /* If some operations fail, we need to skip the remaining bytes
    177          * to put the status in the last byte
    178          */
--> 179         if (to_push - pushed > 0)

What I'm doing here is I'm looking for places where there are signedness
bugs such that "pushed > to_push" and the subtraction results in a high
positive value instead of a negative value.

I think there are potential issues here.  It's definitely not clear
without reading a lot of context whether this code is safe.

    180                 vringh_kiov_advance(&vq->in_iov, to_push - pushed);
    181 
    182         /* Last byte is the status */
    183         bytes = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov, &status, 1);
    184         if (bytes != 1)
    185                 return false;
    186 
    187         pushed += bytes;
    188 
    189         /* Make sure data is wrote before advancing index */
    190         smp_wmb();
    191 
    192         vringh_complete_iotlb(&vq->vring, vq->head, pushed);
    193 
    194         return true;
    195 }

regards,
dan carpenter
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 4+ messages in thread