[Libguestfs] [libnbd PATCH] golang: Simplify nbd_block_status callback array

[Eric Blake]

On Thu, Nov 25, 2021 at 10:50:03AM +0200, Nir Soffer wrote:
> On Wed, Nov 24, 2021 at 10:03 PM Eric Blake <eblake at redhat.com> wrote:
> >
> > Instead of copying from a C uint32_t[] to a golang []uint32, we can
> > exploit the fact that their underlying memory has the same
> > representation.  An unsafe cast to more memory than necessary exposes
> > all the more that Go then needs to give a well-bounded slice, with no
> > copying necessary.
> 
> Nice addition, I did know that we do this extra unneeded copy, and it will be
> nice to avoid it.
> 
> But do we document that the caller must copy the returned entries
> array if they need
> to keep it after the extent callback returns?

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.

> 
> This change may cause use after free in caller code if the code is not
> careful about
> that. I think a warning in the ExtentCallback wrapper would be useful.
> 
> This is the documentation we have now:
> 
>     https://pkg.go.dev/libguestfs.org/libnbd#ExtentCallback
> 
> >
> > https://newbedev.com/pass-struct-and-array-of-structs-to-c-function-from-go
> 
> Maybe a better link is:
> https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices

Yes, that's a more canonical link.

> 
> It also explains a nicer way with Go 1.17 which we can use in a future release.
> 
> > ---
> >  generator/GoLang.ml | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/generator/GoLang.ml b/generator/GoLang.ml
> > index eb3aa26..7455dde 100644
> > --- a/generator/GoLang.ml
> > +++ b/generator/GoLang.ml
> > @@ -1,6 +1,6 @@
> >  (* hey emacs, this is OCaml code: -*- tuareg -*- *)
> >  (* nbd client library in userspace: generator
> > - * Copyright (C) 2013-2020 Red Hat Inc.
> > + * Copyright (C) 2013-2021 Red Hat Inc.
> >   *
> >   * This library is free software; you can redistribute it and/or
> >   * modify it under the terms of the GNU Lesser General Public
> > @@ -513,13 +513,14 @@ import \"unsafe\"
> >
> >  /* Closures. */
> >
> > -func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
> > -    ret := make([]uint32, int (count))
> > -    for i := 0; i < int (count); i++ {
> > -       entry := (*C.uint32_t) (unsafe.Pointer(uintptr(unsafe.Pointer(entries)) + (unsafe.Sizeof(*entries) * uintptr(i))))

The old code is still a bunch of nasty casting.  Maybe the compromise is...

> > -       ret[i] = uint32 (*entry)
> > -    }
> > -    return ret
> > +func use_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
> > +    /* https://stackoverflow.com/questions/48756732/what-does-1-30c-yourtype-do-exactly-in-cgo */
> > +    unsafePtr := unsafe.Pointer(entries)
> > +    /* Max structured reply payload is 64M, so this array size is more than
> > +     * sufficient for the underlying slice we want to expose.
> > +     */
> 
> Why 64M?

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).

> 
> I guess you calculate using maximum request length (2**31 - 1) and
> minimum block size (512)

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

> so we can get up to 8388607 extents per request, with 8 bytes per
> extent - 67108856 bytes.
> If this is the reason, maybe we should include it in
> https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md

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.

> 
> > +    arrayPtr := (*[1 << 20]uint32)(unsafePtr)
> > +    return arrayPtr[:count:count]

...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.

> >  }
> >  ";
> >
> > @@ -601,7 +602,7 @@ func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
> 
> This function is called copy_...

Sounds like it needs to remain a copy, rather than a use.

> 
> >            if !comma then pr ", "; comma := true;
> >            match cbarg with
> >            | CBArrayAndLen (UInt32 n, count) ->
> > -             pr "copy_uint32_array (%s, %s)" n count
> > +             pr "use_uint32_array (%s, %s)" n count
> 
> But we implement by use_... - this may confuse someone in the future,
> and lead to use after free.
> 
> >            | CBBytesIn (n, len) ->
> >               pr "C.GoBytes (%s, C.int (%s))" n len
> >            | CBInt n ->
> > --
> > 2.33.1
> 
> Otherwise it looks good to me.

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