On Thu, Nov 25, 2021 at 10:50:03AM +0200, Nir Soffer wrote: Oh. Ewwwww. I played with this more, and we DO have existing go unit tests that assume you can do an array assignment that survives past the callback: golang/libnbd_460_block_status_test.go: var entries []uint32 func mcf(metacontext string, offset uint64, e []uint32, error *int) int { if *error != 0 { panic("expected *error == 0") } if metacontext == "base:allocation" { entries = e } return 0 } With this patch alone applied, that test still passes, but only by luck. Our current state machine leaves h->bs_entries allocated until the next NBD_CMD_BLOCK_STATUS reply is received over the wire. But when I changed The C code state machine to free(h->bs_entries) as soon as the callback returned control to the C code, then I definitely saw the effects of the use-after-free when the rest of the Go code then saw random contents in the memory because the heap had already been repurposed. In other words, with a copy, our unit test is robust: forcing a copy causes Go to manage the array (and Go's lifetime management of the copy prevents use-after-free); but exposing an alias as in this patch puts the Go code at risk of when the C code then manipulates that array (our unit test was lucky in that it did not queue up back-to-back NBD_CMD_BLOCK_STATUS where consecutive replies from the server could interact badly, but that's not out of the realm of possibilities for a more highly-threaded client use). I don't know if there is more idiomatic Go code for forcing a copy of a transient slice for use in our glue code, but the user's Go code (including our testsuite) shouldn't have to be worried about non-idiomatic lifetimes like that. So this patch as-is is wrong. Yes, that's a more canonical link. The old code is still a bunch of nasty casting. Maybe the compromise is... Because that's the maximum payload we allow from the server for any response to NBD_CMD_READ or NBD_CMD_BLOCK_STATUS. Anything larger and the server is assumed to either be malicious (attempting a denial of service on the client) or we have lost communication sync with the server. Most servers won't even reach that many extents in a single reply (for example, qemu-nbd caps responses to 1M, which is at most 128k extents; see qemu/nbd/server.c NBD_MAX_BLOCK_STATUS_EXTENTS). 512-byte extents is the recommended minimum block size for an extent, but not a hard limit. Rich and I found that it was fairly easy to write an nbdkit plugin that would attempt to send more than 64M of extent information in one reply by sending alternating extents per byte (yes, sending extents that small becomes an amplification attack); and fixed it by capping the maximum extents nbdkit is willing to reply with: https://gitlab.com/nbdkit/nbdkit/-/commit/6e0dc839 Both qemu and nbdkit as servers cap responses, and I don't know of any other servers that implement block status yet (upstream nbd-server does not). But yes, maybe it is worth an NBD spec tweak to explicitly mention that servers should consider capping their responses. ...using the unsafePtr and arrayPtr magic for creating a nice alias variable, but then STILL performing a manual copy into Go-managed memory, and returning the copy (which is now subject to Go lifetime and garbage collection, and therefore no longer a risk of use-after-free) rather than our unsafe alias. Sounds like it needs to remain a copy, rather than a use. Thanks for the review pointing me in the right direction. I'm not yet sure if I will drop this entirely, or try a v2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org