On Mon, Sep 25, 2023 at 02:22:31PM -0500, Eric Blake wrote: > Upcoming additions to support NBD 64-bit effect lengths allow for the > possibility to distinguish between payload length (capped at 32M) and > effect length (64 bits, although we generally assume 63 bits because > of off_t limitations). [...]
> +++ b/nbd/server.c > @@ -2322,9 +2322,11 @@ static int coroutine_fn > nbd_co_receive_request(NBDRequestData *req, > Error **errp) > { > NBDClient *client = req->client; > + bool extended_with_payload; > bool check_length = false; > bool check_rofs = false; > bool allocate_buffer = false; > + bool payload_okay = false; > unsigned payload_len = 0; Pre-existing type mismatch caught as a result of Vladimir's review of 12/12, but: > int valid_flags = NBD_CMD_FLAG_FUA; > int ret; > @@ -2338,6 +2340,13 @@ static int coroutine_fn > nbd_co_receive_request(NBDRequestData *req, > > trace_nbd_co_receive_request_decode_type(request->cookie, request->type, > nbd_cmd_lookup(request->type)); > + extended_with_payload = client->mode >= NBD_MODE_EXTENDED && > + request->flags & NBD_CMD_FLAG_PAYLOAD_LEN; > + if (extended_with_payload) { > + payload_len = request->len; this can assign a 64-bit number into a 32-bit variable, which can truncate to 0,... > + check_length = true; > + } > + > switch (request->type) { > case NBD_CMD_DISC: > /* Special case: we're going to disconnect without a reply, > @@ -2354,6 +2363,15 @@ static int coroutine_fn > nbd_co_receive_request(NBDRequestData *req, > break; > > case NBD_CMD_WRITE: > + if (client->mode >= NBD_MODE_EXTENDED) { > + if (!extended_with_payload) { > + /* The client is noncompliant. Trace it, but proceed. */ > + trace_nbd_co_receive_ext_payload_compliance(request->from, > + request->len); > + } > + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; > + } > + payload_okay = true; > payload_len = request->len; ...the pre-existing code is safe only as long as request->len cannot exceed 32 bytes (which it can't do until later in this series actually enables extended requests). Switching the type now is prudent... > check_length = true; > allocate_buffer = true; > @@ -2395,6 +2413,14 @@ static int coroutine_fn > nbd_co_receive_request(NBDRequestData *req, > request->len, NBD_MAX_BUFFER_SIZE); > return -EINVAL; > } > + if (payload_len && !payload_okay) { > + /* > + * For now, we don't support payloads on other commands; but > + * we can keep the connection alive by ignoring the payload. > + */ > + assert(request->type != NBD_CMD_WRITE); > + request->len = 0; ...otherwise, this check is bypassed for a request size of exactly 4G if check_length is false and thus the previous conditional for request->len vs. NBD_MAX_BUFFER_SIZE didn't trigger (prior to this patch, payload_len was only set for CND_WRITE which also set check_length). Thus, I'm squashing in: diff --git i/nbd/server.c w/nbd/server.c index 5258064e5ac..1cb66e86a89 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -2327,7 +2327,7 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, bool check_rofs = false; bool allocate_buffer = false; bool payload_okay = false; - unsigned payload_len = 0; + uint64_t payload_len = 0; int valid_flags = NBD_CMD_FLAG_FUA; int ret; -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs