Re: [Libguestfs] [libnbd PATCH v3 01/22] block_status: Refactor array storage

On 5/25/23 15:00, Eric Blake wrote: > For 32-bit block status, we were able to cheat and use an array with > an odd number of elements, with array[0] holding the context id, and > passing &array[1] to the user's callback. But once we have 64-bit > extents, we can no longer abuse array element 0 like that, for two > reasons: 64-bit extents contain uint64_t which might not be > alignment-compatible with an array of uint32_t on all architectures, > and the new NBD_REPLY_TYPE_BLOCK_STATUS_EXT adds an additional count > field before the array. > > +++ b/generator/states-reply-structured.c > @@ -126,19 +126,10 @@ REPLY.STRUCTURED_REPLY.CHECK: > length < 12 || ((length-4) & 7) != 0) This is important (the context doesn't show it in full): we're under NBD_REPLY_TYPE_BLOCK_STATUS here (nested under REPLY.STRUCTURED_REPLY.CHECK), and we enforce that length = be32toh (h->sbuf.sr.structured_reply.length); contains the context_id (4 bytes), plus a positive integral number of block descriptor structures (8 bytes each).

> @@ -445,15 +468,16 @@ REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES: > assert (h->bs_entries); > assert (length >= 12); > assert (h->meta_valid); > + count = (length - sizeof h->sbuf.sr.payload.bs_hdr) / sizeof *h->bs_entries; I have a slight problem with the pre-patch code here. We keep the existent assertions (good), but I think the pre-patch RECV_BS_ENTRIES code misses an assertion. Namely, after the size check (i.e., 12+ bytes), the pre-patch code should have said assert ((length-4) & 7) == 0); emphasizing the explicit check under REPLY.STRUCTURED_REPLY.CHECK. The pre-patch code relies on this (a) silently by expecting (length/4) to be an integer (in the mathematical sense), and (b) very silently by expecting (length/4) to be an *odd* integer >= 3.

Here's what I suggest as an update for this patch (to be squashed): diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index da1e46929cd0..6cd4a49baa26 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -43,6 +43,20 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length, return true; } +static bool +bs_reply_length_ok (uint32_t length) +{ + if (length < (sizeof (struct nbd_structured_reply_block_status_hdr) + + sizeof (struct nbd_block_descriptor))) + return false; + + length -= sizeof (struct nbd_structured_reply_block_status_hdr); + if (length % sizeof (struct nbd_block_descriptor) != 0) + return false; + + return true; +}

+ STATE_MACHINE { REPLY.STRUCTURED_REPLY.START: /* We've only read the simple_reply. The structured_reply is longer, @@ -123,7 +137,7 @@ REPLY.STRUCTURED_REPLY.CHECK: case NBD_REPLY_TYPE_BLOCK_STATUS: if (cmd->type != NBD_CMD_BLOCK_STATUS || - length < 12 || ((length-4) & 7) != 0) + !bs_reply_length_ok (length)) goto resync;

@@ -466,8 +480,11 @@ REPLY.STRUCTURED_REPLY.RECV_BS_ENTRIES: assert (cmd->type == NBD_CMD_BLOCK_STATUS); assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent)); assert (h->bs_entries); - assert (length >= 12); + assert (bs_reply_length_ok (length)); assert (h->meta_valid); + STATIC_ASSERT ((sizeof (struct nbd_block_descriptor) % + sizeof *h->bs_entries) == 0, + _block_desc_is_multiple_of_bs_entry); count = (length - sizeof h->sbuf.sr.payload.bs_hdr) / sizeof *h->bs_entries;