| Message ID | c988ccf62ad4000775d3b322744ea0f5900daaa7.1485878688.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
On 01/31/2017 10:09 AM, Alberto Garcia wrote: > The type of qiov->size is size_t but the QEMU I/O code uses int all > over the place to measure request sizes. Overflows are likely going > to be detected by any of the many assert(qiov->size == nbytes), where > nbytes is an int, but let's add proper checks in the QEMUIOVector > initialization, which is where it belongs. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > util/iov.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On 31.01.2017 17:09, Alberto Garcia wrote: > The type of qiov->size is size_t but the QEMU I/O code uses int all > over the place to measure request sizes. Overflows are likely going > to be detected by any of the many assert(qiov->size == nbytes), where > nbytes is an int, but let's add proper checks in the QEMUIOVector > initialization, which is where it belongs. I'm not entirely convinced that's where it belongs. In my opinion, the BlockBackend functions should all get uint64_t size parameters and let blk_check_byte_request() do the rest. I wouldn't mind too much if blk_check_byte_requests() did assertions instead of returning -EIO. But the thing with the I/O vector is that it's not exclusively used for the block layer. I can see at least hw/9pfs/9p.c and hw/usb/core.c using it internally. The assertion probably makes sense even for them, considering that size_t does not have a constant size. But I'm not entirely sold that the I/O vector creation is actually the place where the assertions belong. Max > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > util/iov.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/util/iov.c b/util/iov.c > index 74e6ca8ed7..6b5cc9203c 100644 > --- a/util/iov.c > +++ b/util/iov.c > @@ -283,13 +283,18 @@ void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov) > qiov->niov = niov; > qiov->nalloc = -1; > qiov->size = 0; > - for (i = 0; i < niov; i++) > + for (i = 0; i < niov; i++) { > + assert(iov[i].iov_len <= INT_MAX); > + assert(qiov->size <= INT_MAX - iov[i].iov_len); > qiov->size += iov[i].iov_len; > + } > } > > void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len) > { > assert(qiov->nalloc != -1); > + assert(len <= INT_MAX); > + assert(qiov->size <= INT_MAX - len); > > if (qiov->niov == qiov->nalloc) { > qiov->nalloc = 2 * qiov->nalloc + 1; >
On Wed 01 Feb 2017 10:51:01 PM CET, Max Reitz <mreitz@redhat.com> wrote: > The assertion probably makes sense even for them, considering that > size_t does not have a constant size. But I'm not entirely sold that > the I/O vector creation is actually the place where the assertions > belong. I was actually just reading virtio_blk_handle_request() and I see that the I/O vector is initialized before the check for the maximum request size that returns VIRTIO_BLK_S_IOERR. So you're probably right and it's safer not to add that assertion. Berto
On 01.02.2017 22:55, Alberto Garcia wrote: > On Wed 01 Feb 2017 10:51:01 PM CET, Max Reitz <mreitz@redhat.com> wrote: > >> The assertion probably makes sense even for them, considering that >> size_t does not have a constant size. But I'm not entirely sold that >> the I/O vector creation is actually the place where the assertions >> belong. > > I was actually just reading virtio_blk_handle_request() and I see that > the I/O vector is initialized before the check for the maximum request > size that returns VIRTIO_BLK_S_IOERR. > > So you're probably right and it's safer not to add that assertion. So... Apply patch 1 and hope we do everything right in the future? :-) Max
On Wed 01 Feb 2017 10:56:29 PM CET, Max Reitz <mreitz@redhat.com> wrote: >> I was actually just reading virtio_blk_handle_request() and I see >> that the I/O vector is initialized before the check for the maximum >> request size that returns VIRTIO_BLK_S_IOERR. >> >> So you're probably right and it's safer not to add that assertion. > > So... Apply patch 1 and hope we do everything right in the future? :-) I think patch 1 is definitely necessary and enough to solve the crash that I reported. Tomorrow I'll think about what to do in place of the second one :-) Berto
diff --git a/util/iov.c b/util/iov.c index 74e6ca8ed7..6b5cc9203c 100644 --- a/util/iov.c +++ b/util/iov.c @@ -283,13 +283,18 @@ void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov) qiov->niov = niov; qiov->nalloc = -1; qiov->size = 0; - for (i = 0; i < niov; i++) + for (i = 0; i < niov; i++) { + assert(iov[i].iov_len <= INT_MAX); + assert(qiov->size <= INT_MAX - iov[i].iov_len); qiov->size += iov[i].iov_len; + } } void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len) { assert(qiov->nalloc != -1); + assert(len <= INT_MAX); + assert(qiov->size <= INT_MAX - len); if (qiov->niov == qiov->nalloc) { qiov->nalloc = 2 * qiov->nalloc + 1;
The type of qiov->size is size_t but the QEMU I/O code uses int all over the place to measure request sizes. Overflows are likely going to be detected by any of the many assert(qiov->size == nbytes), where nbytes is an int, but let's add proper checks in the QEMUIOVector initialization, which is where it belongs. Signed-off-by: Alberto Garcia <berto@igalia.com> --- util/iov.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)