On 12.08.26 13:29, Denis V. Lunev wrote:
Cookies are converted into indices of s->requests[] in several places
and the result is used right away, without any check:
int i = COOKIE_TO_INDEX(cookie);
...
return nbd_co_receive_offset_data_payload(s, s->requests[i].offset,
COOKIE_TO_INDEX() subtracts one, so a zero cookie becomes an index of
-1 and the access lands in front of the array. This is undefined and,
depending on the type of the index and on what the compiler has put
there, it can as well pass silently: at the site above i is signed, so
even a plain i < MAX_NBD_REQUESTS check would happily let -1 through.
The only cookie which is checked today is the one taken from the wire,
in nbd_receive_replies(), where an invalid value is a protocol error
rather than an internal inconsistency and thus has to stay a channel
error. Route every other conversion through a helper which asserts the
range before it returns the slot, so that the check can not be
forgotten again.
The cookie of the reply in flight goes through the helper as well. It
is not an unchecked value: it has either passed the check above, or it
has been cleared by the previous commit, so a stale index there is an
internal inconsistency too.
Signed-off-by: Denis V. Lunev <[email protected]>
Cc: Eric Blake <[email protected]>
Cc: Vladimir Sementsov-Ogievskiy <[email protected]>
---
block/nbd.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index d9b776283f..21f0f9d40d 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -136,6 +136,16 @@ static void nbd_clear_bdrvstate(BlockDriverState *bs)
s->x_dirty_bitmap = NULL;
}
+/* Not for cookies coming from the wire, those are checked separately. */
Still, I think, if we do such a wrapper, better to call it always with no
exclusions.
For example, add errp, and pass &error_abort when don't expect an error.
+static NBDClientRequest *nbd_request_by_cookie(BDRVNBDState *s, uint64_t
cookie)
+{
+ uint64_t ind = COOKIE_TO_INDEX(cookie);
+
+ assert(ind < MAX_NBD_REQUESTS);
+
+ return &s->requests[ind];
+}
+
/* Called with s->receive_mutex taken. */
static bool coroutine_fn nbd_recv_coroutine_wake_one(NBDClientRequest *req)
{
@@ -422,7 +432,8 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState
*s, uint64_t cookie,
Error **errp)
{
int ret;
- uint64_t ind = COOKIE_TO_INDEX(cookie), ind2;
+ NBDClientRequest *req = nbd_request_by_cookie(s, cookie);
+ uint64_t ind2;
QEMU_LOCK_GUARD(&s->receive_mutex);
while (true) {
@@ -437,10 +448,9 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState
*s, uint64_t cookie,
* woken by whoever set s->reply.cookie (or never wait in this
* yield). So, we should not wake it here.
*/
- ind2 = COOKIE_TO_INDEX(s->reply.cookie);
- assert(!s->requests[ind2].receiving);
+ assert(!nbd_request_by_cookie(s, s->reply.cookie)->receiving);
- s->requests[ind].receiving = true;
+ req->receiving = true;
qemu_co_mutex_unlock(&s->receive_mutex);
qemu_coroutine_yield();
@@ -454,7 +464,7 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState
*s, uint64_t cookie,
*/
qemu_co_mutex_lock(&s->receive_mutex);
- assert(!s->requests[ind].receiving);
+ assert(!req->receiving);
continue;
}
@@ -861,7 +871,6 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
{
ERRP_GUARD();
int ret;
- int i = COOKIE_TO_INDEX(cookie);
void *local_payload = NULL;
NBDStructuredReplyChunk *chunk;
@@ -919,8 +928,8 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
return -EINVAL;
}
- return nbd_co_receive_offset_data_payload(s, s->requests[i].offset,
- qiov, errp);
+ return nbd_co_receive_offset_data_payload(
+ s, nbd_request_by_cookie(s, cookie)->offset, qiov, errp);
}
if (nbd_reply_type_is_error(chunk->type)) {
@@ -1067,7 +1076,7 @@ static bool coroutine_fn
nbd_reply_chunk_iter_receive(BDRVNBDState *s,
break_loop:
qemu_mutex_lock(&s->requests_lock);
- s->requests[COOKIE_TO_INDEX(cookie)].coroutine = NULL;
+ nbd_request_by_cookie(s, cookie)->coroutine = NULL;
s->in_flight--;
qemu_co_queue_next(&s->free_sema);
qemu_mutex_unlock(&s->requests_lock);
--
Best regards,
Vladimir
