On Wed, Apr 23, 2025 at 08:01:55AM -0500, Eric Blake via Libguestfs wrote: My additional analysis:
> > > > blocks[i].length = length = MIN (e.length, UINT32_MAX); > > > > The extent we're sending has e.length is >= 4G, which we truncate to > > UINT32_MAX == 4G-1. The ONLY time length < e.length is if we truncated an extent that was 4G or larger to 4G-1. All other times, length == e.length. > > > > pos += length; > > > > pos += 4G-1 If there was only one extent, then pos == offset+length; if there was more than one extent, then pos > offset+length. We may still have pos < offset+count, depending on whether the extents seen so far have summed up past count. > > > > In the previous code: > > > > if (pos > offset + count) /* this must be the last block */ Because we only accept a 32-bit count, the largest value we can see is a count of 4G-1. If there was more than one extent (even a one-byte extent followed by a 4G-1 extent), we are guaranteed that pos > offset+count. But the NBD protocol says that the last extent in our reply must start prior to offset+count (when req_one is off, our last extent may extend beyond the client's limit, but no new extents should be volunteered to the client). > > break; > > > > branch not taken so we reach: > > > > assert (e.length <= length); > > > > e.length == 4G, length == 4G-1, so we assert here. > > Yep. And if compiled with NDEBUG, the assertion doesn't trigger, but > the loop could continue and try to send the client an extent that > starts after the maximum offset permitted, so even if we don't crash, > we'd be violating the NBD spec. > > > > > In the updated code, the assert is avoided, which means this branch > > must now be taken, which means the client requested 'offset' == 0 & > > 'count' must be exactly 4G-1, I think? (Or any relative request) > > > > if (pos >= offset + count) /* this must be the last block */ > > break; > > The combination of user requesting len=4G-1 and the plugin answering > with a single extent >= 4G is what I tested. I still need to analyze > if there are other conditions where the off-by-one could also bite us > where we provide too many extents in reply to the user without > triggering the assertion. But it looks like the assertion can only be > triggered if the user requests len 4G-1. I could not find any other ways to tickle the assertion failure (any split of the plugin's report into more than one extent will either break the loop, or satisfy the assertion); but the following DOES coerce the server into sending too many extents in violation of the protocol without triggering the assertion: $ ext=" echo 0 1 0 echo 1 $((4*1024*1024*1024-1)) 1 echo $((4*1024*1024*1024)) 1G 0 " $ nbdkit -f eval --filter=ddrescue get_size='echo 5G' extents="$ext" \ --run 'nbdsh --base-allocation --uri "$uri" -c " def f(m,o,e,s): print(e) h.block_status(2**32-1,1,f) "' [4294967295, 1, 1073741824, 0] and is also fixed by my patch to correctly print [4294967295, 1] I'll update the test to additionally cover that in v2 of the patch, while still waiting for secalert's answer on a CVE. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org _______________________________________________ Libguestfs mailing list -- guestfs@lists.libguestfs.org To unsubscribe send an email to guestfs-le...@lists.libguestfs.org
