Dan Carpenter
2021-Sep-29 11:37 UTC
[bug report] vdpa_sim_blk: implement ramdisk behaviour
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
Dan Carpenter
2021-Sep-29 11:46 UTC
[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