Eric Blake
2022-Feb-09 22:07 UTC
[Libguestfs] [libnbd PATCH 3/3] api: Add new API nbd_set_pread_initialize()
The recent patch series for CVE-2022-0485 demonstrated that when
applications using libnbd are not careful about error checking, the
difference on whether a data leak is at least sanitized (all zeroes,
partial reads, or data leftover from a prior read) vs. a dangerous
information leak (uninitialized data from the heap) was partly under
libnbd's control. The previous two patches changed libnbd to always
sanitize, as a security hardening technique that prevents heap leaks
no matter how buggy the client app is. But a blind memset() also adds
an execution delay, even if it doesn't show up as the hot spot in our
profiling to the slower time spent with network traffic.
At any rate, if client apps choose to pre-initialize their buffers, or
otherwise audit their code to take on their own risk about not
dereferencing a buffer on failure paths, then the time spent by libnbd
doing memset() is wasted; so it is worth adding a knob to let a user
opt in to faster execution at the expense of giving up our memset()
hardening on their behalf.
---
lib/internal.h | 5 +-
generator/API.ml | 79 +++++++++++++++++++---
generator/C.ml | 3 +-
lib/handle.c | 17 ++++-
python/t/110-defaults.py | 3 +-
python/t/120-set-non-defaults.py | 4 +-
ocaml/tests/test_110_defaults.ml | 4 +-
ocaml/tests/test_120_set_non_defaults.ml | 5 +-
tests/errors.c | 25 ++++++-
golang/libnbd_110_defaults_test.go | 10 ++-
golang/libnbd_120_set_non_defaults_test.go | 12 ++++
11 files changed, 148 insertions(+), 19 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index 0e205aba..525499a9 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -1,5 +1,5 @@
/* nbd client library in userspace: internal definitions
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 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
@@ -123,6 +123,9 @@ struct nbd_handle {
/* Full info mode. */
bool full_info;
+ /* Sanitization for pread. */
+ bool pread_initialize;
+
/* Global flags from the server. */
uint16_t gflags;
diff --git a/generator/API.ml b/generator/API.ml
index 98f90031..9b7eb545 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -778,6 +778,44 @@ "get_handshake_flags", {
Link "aio_is_created"; Link
"aio_is_ready"];
};
+ "set_pread_initialize", {
+ default_call with
+ args = [Bool "request"]; ret = RErr;
+ shortdesc = "control whether libnbd pre-initializes read
buffers";
+ longdesc = "\
+By default, libnbd will pre-initialize the contents of a buffer
+passed to calls such as L<nbd_pread(3)> to all zeroes prior to checking
+for any other errors, so that even if a client application passed in an
+uninitialized buffer but fails to check for errors, it will not result
+in a potential security risk caused by an accidental leak of prior heap
+contents. However, for a client application that has audited that an
+uninitialized buffer is never dereferenced, or which performs its own
+pre-initialization, libnbd's sanitization efforts merely pessimize
+performance.
+
+Calling this function with C<request> set to false tells libnbd to
+skip the buffer initialization step in read commands.";
+ see_also = [Link "get_pread_initialize";
+ Link "set_strict_mode";
+ Link "pread"; Link "pread_structured"; Link
"aio_pread";
+ Link "aio_pread_structured"];
+ };
+
+ "get_pread_initialize", {
+ default_call with
+ args = []; ret = RBool;
+ may_set_error = false;
+ shortdesc = "see whether libnbd pre-initializes read buffers";
+ longdesc = "\
+Return whether libnbd performs a pre-initialization of a buffer passed
+to L<nbd_pread(3)> and similar to all zeroes, as set by
+L<nbd_set_pread_initialize(3)>.";
+ see_also = [Link "set_pread_initialize";
+ Link "set_strict_mode";
+ Link "pread"; Link "pread_structured"; Link
"aio_pread";
+ Link "aio_pread_structured"];
+ };
+
"set_strict_mode", {
default_call with
args = [ Flags ("flags", strict_flags) ]; ret = RErr;
@@ -1831,11 +1869,16 @@ "pread", {
The C<flags> parameter must be C<0> for now (it exists for future
NBD
protocol extensions).
-Note that if this command fails, it is unspecified whether the contents
-of C<buf> will read as zero or as partial results from the server."
+Note that if this command fails, and L<nbd_get_pread_initialize(3)>
+returns true, then libnbd sanitized C<buf>, but it is unspecified
+whether the contents of C<buf> will read as zero or as partial results
+from the server. If L<nbd_get_pread_initialize(3)> returns false,
+then libnbd did not sanitize C<buf>, and the contents are undefined
+on failure."
^ strict_call_description;
see_also = [Link "aio_pread"; Link "pread_structured";
- Link "get_block_size"; Link
"set_strict_mode"];
+ Link "get_block_size"; Link
"set_strict_mode";
+ Link "set_pread_initialize"];
example = Some "examples/fetch-first-sector.c";
};
@@ -1919,8 +1962,12 @@ "pread_structured", {
this, see L<nbd_can_df(3)>). Libnbd does not validate that the server
actually obeys the flag.
-Note that if this command fails, it is unspecified whether the contents
-of C<buf> will read as zero or as partial results from the server."
+Note that if this command fails, and L<nbd_get_pread_initialize(3)>
+returns true, then libnbd sanitized C<buf>, but it is unspecified
+whether the contents of C<buf> will read as zero or as partial results
+from the server. If L<nbd_get_pread_initialize(3)> returns false,
+then libnbd did not sanitize C<buf>, and the contents are undefined
+on failure."
^ strict_call_description;
see_also = [Link "can_df"; Link "pread";
Link "aio_pread_structured"; Link
"get_block_size";
@@ -2463,8 +2510,13 @@ "aio_pread", {
Note that you must ensure C<buf> is valid until the command has
completed. Furthermore, if the C<error> parameter to
C<completion_callback> is set or if L<nbd_aio_command_completed(3)>
-reports failure, it is unspecified whether the contents
-of C<buf> will read as zero or as partial results from the server.
+reports failure, and if L<nbd_get_pread_initialize(3)> returns true,
+then libnbd sanitized C<buf>, but it is unspecified whether the
+contents of C<buf> will read as zero or as partial results from the
+server. If L<nbd_get_pread_initialize(3)> returns false, then
+libnbd did not sanitize C<buf>, and the contents are undefined
+on failure.
+
Other parameters behave as documented in L<nbd_pread(3)>."
^ strict_call_description;
example = Some "examples/aio-connect-read.c";
@@ -2492,8 +2544,13 @@ "aio_pread_structured", {
Note that you must ensure C<buf> is valid until the command has
completed. Furthermore, if the C<error> parameter to
C<completion_callback> is set or if L<nbd_aio_command_completed(3)>
-reports failure, it is unspecified whether the contents
-of C<buf> will read as zero or as partial results from the server.
+reports failure, and if L<nbd_get_pread_initialize(3)> returns true,
+then libnbd sanitized C<buf>, but it is unspecified whether the
+contents of C<buf> will read as zero or as partial results from the
+server. If L<nbd_get_pread_initialize(3)> returns false, then
+libnbd did not sanitize C<buf>, and the contents are undefined
+on failure.
+
Other parameters behave as documented in
L<nbd_pread_structured(3)>."
^ strict_call_description;
see_also = [SectionLink "Issuing asynchronous commands";
@@ -3136,6 +3193,10 @@ let first_version "get_private_data", (1,
8);
"get_uri", (1, 8);
+ (* Added in 1.11.x development cycle, will be stable and supported in 1.12.
*)
+ "set_pread_initialize", (1, 12);
+ "get_pread_initialize", (1, 12);
+
(* These calls are proposed for a future version of libnbd, but
* have not been added to any released version so far.
"get_tls_certificates", (1, ??);
diff --git a/generator/C.ml b/generator/C.ml
index 4a5bb589..2b6198c3 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -496,7 +496,8 @@ let
function
| BytesOut (n, count)
| BytesPersistOut (n, count) ->
- pr " memset (%s, 0, %s);\n" n count
+ pr " if (h->pread_initialize)\n";
+ pr " memset (%s, 0, %s);\n" n count
| _ -> ()
) args;
diff --git a/lib/handle.c b/lib/handle.c
index cbb37e89..4f00c059 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 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
@@ -64,6 +64,7 @@ nbd_create (void)
h->unique = 1;
h->tls_verify_peer = true;
h->request_sr = true;
+ h->pread_initialize = true;
h->uri_allow_transports = LIBNBD_ALLOW_TRANSPORT_MASK;
h->uri_allow_tls = LIBNBD_TLS_ALLOW;
@@ -393,6 +394,20 @@ nbd_unlocked_get_handshake_flags (struct nbd_handle *h)
return h->gflags;
}
+int
+nbd_unlocked_set_pread_initialize (struct nbd_handle *h, bool request)
+{
+ h->pread_initialize = request;
+ return 0;
+}
+
+/* NB: may_set_error = false. */
+int
+nbd_unlocked_get_pread_initialize (struct nbd_handle *h)
+{
+ return h->pread_initialize;
+}
+
int
nbd_unlocked_set_strict_mode (struct nbd_handle *h, uint32_t flags)
{
diff --git a/python/t/110-defaults.py b/python/t/110-defaults.py
index fb961cfd..a4262dae 100644
--- a/python/t/110-defaults.py
+++ b/python/t/110-defaults.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2020 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -22,5 +22,6 @@ assert h.get_export_name() == ""
assert h.get_full_info() is False
assert h.get_tls() == nbd.TLS_DISABLE
assert h.get_request_structured_replies() is True
+assert h.get_pread_initialize() is True
assert h.get_handshake_flags() == nbd.HANDSHAKE_FLAG_MASK
assert h.get_opt_mode() is False
diff --git a/python/t/120-set-non-defaults.py b/python/t/120-set-non-defaults.py
index 3da0c23e..e71c6ad0 100644
--- a/python/t/120-set-non-defaults.py
+++ b/python/t/120-set-non-defaults.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2020 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -33,6 +33,8 @@ if h.supports_tls():
assert h.get_tls() == nbd.TLS_ALLOW
h.set_request_structured_replies(False)
assert h.get_request_structured_replies() is False
+h.set_pread_initialize(False)
+assert h.get_pread_initialize() is False
try:
h.set_handshake_flags(nbd.HANDSHAKE_FLAG_MASK + 1)
assert False
diff --git a/ocaml/tests/test_110_defaults.ml b/ocaml/tests/test_110_defaults.ml
index b36949f0..04aa744a 100644
--- a/ocaml/tests/test_110_defaults.ml
+++ b/ocaml/tests/test_110_defaults.ml
@@ -1,6 +1,6 @@
(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* libnbd OCaml test case
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 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
@@ -28,6 +28,8 @@ let
assert (tls = NBD.TLS.DISABLE);
let sr = NBD.get_request_structured_replies nbd in
assert (sr = true);
+ let init = NBD.get_pread_initialize nbd in
+ assert (init = true);
let flags = NBD.get_handshake_flags nbd in
assert (flags = NBD.HANDSHAKE_FLAG.mask);
let opt = NBD.get_opt_mode nbd in
diff --git a/ocaml/tests/test_120_set_non_defaults.ml
b/ocaml/tests/test_120_set_non_defaults.ml
index 67928bb5..f9498076 100644
--- a/ocaml/tests/test_120_set_non_defaults.ml
+++ b/ocaml/tests/test_120_set_non_defaults.ml
@@ -1,6 +1,6 @@
(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* libnbd OCaml test case
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 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
@@ -42,6 +42,9 @@ let
NBD.set_request_structured_replies nbd false;
let sr = NBD.get_request_structured_replies nbd in
assert (sr = false);
+ NBD.set_pread_initialize nbd false;
+ let init = NBD.get_pread_initialize nbd in
+ assert (init = false);
(try
NBD.set_handshake_flags nbd [ NBD.HANDSHAKE_FLAG.UNKNOWN 2 ];
assert false
diff --git a/tests/errors.c b/tests/errors.c
index f597b7ee..0298da88 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -213,7 +213,15 @@ main (int argc, char *argv[])
}
- /* Issue a connected command when not connected. */
+ /* Issue a connected command when not connected. pread_initialize defaults
+ * to set.
+ */
+ if (nbd_get_pread_initialize (nbd) != 1) {
+ fprintf (stderr, "%s: test failed: "
+ "nbd_get_pread_initialize gave unexpected result\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
buf[0] = '1';
if (nbd_pread (nbd, buf, 512, 0, 0) != -1) {
fprintf (stderr, "%s: test failed: "
@@ -294,7 +302,14 @@ main (int argc, char *argv[])
}
check (EINVAL, "nbd_aio_command_completed: ");
- /* Read from an invalid offset, client-side */
+ /* Read from an invalid offset, client-side. When pread_initialize is off,
+ * libnbd should not have touched our buffer.
+ */
+ if (nbd_set_pread_initialize (nbd, false) == -1) {
+ fprintf (stderr, "%s\n", nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+ buf[0] = '1';
strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_BOUNDS;
if (nbd_set_strict_mode (nbd, strict) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
@@ -307,6 +322,12 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
check (EINVAL, "nbd_aio_pread: ");
+ if (buf[0] != '1') {
+ fprintf (stderr, "%s: test failed: "
+ "nbd_pread incorrectly sanitized buffer on client-side
error\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
/* We guarantee callbacks will be freed even on all error paths. */
if (nbd_aio_pread_structured (nbd, buf, 512, -1,
diff --git a/golang/libnbd_110_defaults_test.go
b/golang/libnbd_110_defaults_test.go
index b3ceb45d..ca7c1c41 100644
--- a/golang/libnbd_110_defaults_test.go
+++ b/golang/libnbd_110_defaults_test.go
@@ -1,5 +1,5 @@
/* libnbd golang tests
- * Copyright (C) 2013-2021 Red Hat Inc.
+ * Copyright (C) 2013-2022 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
@@ -59,6 +59,14 @@ func Test110Defaults(t *testing.T) {
t.Fatalf("unexpected structured replies state")
}
+ init, err := h.GetPreadInitialize()
+ if err != nil {
+ t.Fatalf("could not get pread initialize state: %s", err)
+ }
+ if init != true {
+ t.Fatalf("unexpected pread initialize state")
+ }
+
flags, err := h.GetHandshakeFlags()
if err != nil {
t.Fatalf("could not get handshake flags: %s", err)
diff --git a/golang/libnbd_120_set_non_defaults_test.go
b/golang/libnbd_120_set_non_defaults_test.go
index f112456c..029f0db3 100644
--- a/golang/libnbd_120_set_non_defaults_test.go
+++ b/golang/libnbd_120_set_non_defaults_test.go
@@ -93,6 +93,18 @@ func Test120SetNonDefaults(t *testing.T) {
t.Fatalf("unexpected structured replies state")
}
+ err = h.SetPreadInitialize(false)
+ if err != nil {
+ t.Fatalf("could not set pread initialize state: %s", err)
+ }
+ init, err := h.GetPreadInitialize()
+ if err != nil {
+ t.Fatalf("could not get pread initialize state: %s", err)
+ }
+ if init != false {
+ t.Fatalf("unexpected pread initialize state")
+ }
+
err = h.SetHandshakeFlags(HANDSHAKE_FLAG_MASK + 1)
if err == nil {
t.Fatalf("expect failure for out-of-range flags")
--
2.34.1
Richard W.M. Jones
2022-Feb-10 09:38 UTC
[Libguestfs] [libnbd PATCH 3/3] api: Add new API nbd_set_pread_initialize()
On Wed, Feb 09, 2022 at 04:07:26PM -0600, Eric Blake wrote:> + "set_pread_initialize", { > + default_call with > + args = [Bool "request"]; ret = RErr; > + shortdesc = "control whether libnbd pre-initializes read buffers"; > + longdesc = "\ > +By default, libnbd will pre-initialize the contents of a buffer > +passed to calls such as L<nbd_pread(3)> to all zeroes prior to checking > +for any other errors, so that even if a client application passed in an > +uninitialized buffer but fails to check for errors, it will not result > +in a potential security risk caused by an accidental leak of prior heap > +contents. However, for a client application that has audited that an > +uninitialized buffer is never dereferenced, or which performs its own > +pre-initialization, libnbd's sanitization efforts merely pessimize > +performance. > + > +Calling this function with C<request> set to false tells libnbd to > +skip the buffer initialization step in read commands."; > + see_also = [Link "get_pread_initialize"; > + Link "set_strict_mode"; > + Link "pread"; Link "pread_structured"; Link "aio_pread"; > + Link "aio_pread_structured"]; > + };Could it be worth mentioning CVE-2022-0485 by name in the text here? And/or linking to: https://listman.redhat.com/archives/libguestfs/2022-February/msg00104.html Anyway the whole patch series looks good, so: Reviewed-by: Richard W.M. Jones <rjones at redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Laszlo Ersek
2022-Feb-10 14:01 UTC
[Libguestfs] [libnbd PATCH 3/3] api: Add new API nbd_set_pread_initialize()
On 02/09/22 23:07, Eric Blake wrote:> The recent patch series for CVE-2022-0485 demonstrated that when > applications using libnbd are not careful about error checking, the > difference on whether a data leak is at least sanitized (all zeroes, > partial reads, or data leftover from a prior read) vs. a dangerous > information leak (uninitialized data from the heap) was partly under > libnbd's control. The previous two patches changed libnbd to always > sanitize, as a security hardening technique that prevents heap leaks > no matter how buggy the client app is. But a blind memset() also adds > an execution delay, even if it doesn't show up as the hot spot in our > profiling to the slower time spent with network traffic. > > At any rate, if client apps choose to pre-initialize their buffers, or > otherwise audit their code to take on their own risk about not > dereferencing a buffer on failure paths, then the time spent by libnbd > doing memset() is wasted; so it is worth adding a knob to let a user > opt in to faster execution at the expense of giving up our memset() > hardening on their behalf. > --- > lib/internal.h | 5 +- > generator/API.ml | 79 +++++++++++++++++++--- > generator/C.ml | 3 +- > lib/handle.c | 17 ++++- > python/t/110-defaults.py | 3 +- > python/t/120-set-non-defaults.py | 4 +- > ocaml/tests/test_110_defaults.ml | 4 +- > ocaml/tests/test_120_set_non_defaults.ml | 5 +- > tests/errors.c | 25 ++++++- > golang/libnbd_110_defaults_test.go | 10 ++- > golang/libnbd_120_set_non_defaults_test.go | 12 ++++ > 11 files changed, 148 insertions(+), 19 deletions(-) > > diff --git a/lib/internal.h b/lib/internal.h > index 0e205aba..525499a9 100644 > --- a/lib/internal.h > +++ b/lib/internal.h > @@ -1,5 +1,5 @@ > /* nbd client library in userspace: internal definitions > - * Copyright (C) 2013-2020 Red Hat Inc. > + * Copyright (C) 2013-2022 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 > @@ -123,6 +123,9 @@ struct nbd_handle { > /* Full info mode. */ > bool full_info; > > + /* Sanitization for pread. */ > + bool pread_initialize; > + > /* Global flags from the server. */ > uint16_t gflags; > > diff --git a/generator/API.ml b/generator/API.ml > index 98f90031..9b7eb545 100644 > --- a/generator/API.ml > +++ b/generator/API.ml > @@ -778,6 +778,44 @@ "get_handshake_flags", { > Link "aio_is_created"; Link "aio_is_ready"]; > }; > > + "set_pread_initialize", { > + default_call with > + args = [Bool "request"]; ret = RErr; > + shortdesc = "control whether libnbd pre-initializes read buffers"; > + longdesc = "\ > +By default, libnbd will pre-initialize the contents of a buffer > +passed to calls such as L<nbd_pread(3)> to all zeroes prior to checking > +for any other errors, so that even if a client application passed in an > +uninitialized buffer but fails to check for errors, it will not result > +in a potential security risk caused by an accidental leak of prior heap > +contents. However, for a client application that has audited that an > +uninitialized buffer is never dereferenced, or which performs its own > +pre-initialization, libnbd's sanitization efforts merely pessimize > +performance. > + > +Calling this function with C<request> set to false tells libnbd to > +skip the buffer initialization step in read commands."; > + see_also = [Link "get_pread_initialize"; > + Link "set_strict_mode"; > + Link "pread"; Link "pread_structured"; Link "aio_pread"; > + Link "aio_pread_structured"]; > + }; > + > + "get_pread_initialize", { > + default_call with > + args = []; ret = RBool; > + may_set_error = false; > + shortdesc = "see whether libnbd pre-initializes read buffers"; > + longdesc = "\ > +Return whether libnbd performs a pre-initialization of a buffer passed > +to L<nbd_pread(3)> and similar to all zeroes, as set by > +L<nbd_set_pread_initialize(3)>."; > + see_also = [Link "set_pread_initialize"; > + Link "set_strict_mode"; > + Link "pread"; Link "pread_structured"; Link "aio_pread"; > + Link "aio_pread_structured"]; > + }; > + > "set_strict_mode", { > default_call with > args = [ Flags ("flags", strict_flags) ]; ret = RErr; > @@ -1831,11 +1869,16 @@ "pread", { > The C<flags> parameter must be C<0> for now (it exists for future NBD > protocol extensions). > > -Note that if this command fails, it is unspecified whether the contents > -of C<buf> will read as zero or as partial results from the server." > +Note that if this command fails, and L<nbd_get_pread_initialize(3)> > +returns true, then libnbd sanitized C<buf>, but it is unspecified > +whether the contents of C<buf> will read as zero or as partial results > +from the server. If L<nbd_get_pread_initialize(3)> returns false, > +then libnbd did not sanitize C<buf>, and the contents are undefined > +on failure." > ^ strict_call_description; > see_also = [Link "aio_pread"; Link "pread_structured"; > - Link "get_block_size"; Link "set_strict_mode"]; > + Link "get_block_size"; Link "set_strict_mode"; > + Link "set_pread_initialize"]; > example = Some "examples/fetch-first-sector.c"; > }; > > @@ -1919,8 +1962,12 @@ "pread_structured", { > this, see L<nbd_can_df(3)>). Libnbd does not validate that the server > actually obeys the flag. > > -Note that if this command fails, it is unspecified whether the contents > -of C<buf> will read as zero or as partial results from the server." > +Note that if this command fails, and L<nbd_get_pread_initialize(3)> > +returns true, then libnbd sanitized C<buf>, but it is unspecified > +whether the contents of C<buf> will read as zero or as partial results > +from the server. If L<nbd_get_pread_initialize(3)> returns false, > +then libnbd did not sanitize C<buf>, and the contents are undefined > +on failure." > ^ strict_call_description; > see_also = [Link "can_df"; Link "pread"; > Link "aio_pread_structured"; Link "get_block_size"; > @@ -2463,8 +2510,13 @@ "aio_pread", { > Note that you must ensure C<buf> is valid until the command has > completed. Furthermore, if the C<error> parameter to > C<completion_callback> is set or if L<nbd_aio_command_completed(3)> > -reports failure, it is unspecified whether the contents > -of C<buf> will read as zero or as partial results from the server. > +reports failure, and if L<nbd_get_pread_initialize(3)> returns true, > +then libnbd sanitized C<buf>, but it is unspecified whether the > +contents of C<buf> will read as zero or as partial results from the > +server. If L<nbd_get_pread_initialize(3)> returns false, then > +libnbd did not sanitize C<buf>, and the contents are undefined > +on failure. > + > Other parameters behave as documented in L<nbd_pread(3)>." > ^ strict_call_description; > example = Some "examples/aio-connect-read.c"; > @@ -2492,8 +2544,13 @@ "aio_pread_structured", { > Note that you must ensure C<buf> is valid until the command has > completed. Furthermore, if the C<error> parameter to > C<completion_callback> is set or if L<nbd_aio_command_completed(3)> > -reports failure, it is unspecified whether the contents > -of C<buf> will read as zero or as partial results from the server. > +reports failure, and if L<nbd_get_pread_initialize(3)> returns true, > +then libnbd sanitized C<buf>, but it is unspecified whether the > +contents of C<buf> will read as zero or as partial results from the > +server. If L<nbd_get_pread_initialize(3)> returns false, then > +libnbd did not sanitize C<buf>, and the contents are undefined > +on failure. > + > Other parameters behave as documented in L<nbd_pread_structured(3)>." > ^ strict_call_description; > see_also = [SectionLink "Issuing asynchronous commands"; > @@ -3136,6 +3193,10 @@ let first_version > "get_private_data", (1, 8); > "get_uri", (1, 8); > > + (* Added in 1.11.x development cycle, will be stable and supported in 1.12. *) > + "set_pread_initialize", (1, 12); > + "get_pread_initialize", (1, 12); > + > (* These calls are proposed for a future version of libnbd, but > * have not been added to any released version so far. > "get_tls_certificates", (1, ??); > diff --git a/generator/C.ml b/generator/C.ml > index 4a5bb589..2b6198c3 100644 > --- a/generator/C.ml > +++ b/generator/C.ml > @@ -496,7 +496,8 @@ let > function > | BytesOut (n, count) > | BytesPersistOut (n, count) -> > - pr " memset (%s, 0, %s);\n" n count > + pr " if (h->pread_initialize)\n"; > + pr " memset (%s, 0, %s);\n" n count > | _ -> () > ) args; > > diff --git a/lib/handle.c b/lib/handle.c > index cbb37e89..4f00c059 100644 > --- a/lib/handle.c > +++ b/lib/handle.c > @@ -1,5 +1,5 @@ > /* NBD client library in userspace > - * Copyright (C) 2013-2020 Red Hat Inc. > + * Copyright (C) 2013-2022 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 > @@ -64,6 +64,7 @@ nbd_create (void) > h->unique = 1; > h->tls_verify_peer = true; > h->request_sr = true; > + h->pread_initialize = true; > > h->uri_allow_transports = LIBNBD_ALLOW_TRANSPORT_MASK; > h->uri_allow_tls = LIBNBD_TLS_ALLOW; > @@ -393,6 +394,20 @@ nbd_unlocked_get_handshake_flags (struct nbd_handle *h) > return h->gflags; > } > > +int > +nbd_unlocked_set_pread_initialize (struct nbd_handle *h, bool request) > +{ > + h->pread_initialize = request; > + return 0; > +} > + > +/* NB: may_set_error = false. */ > +int > +nbd_unlocked_get_pread_initialize (struct nbd_handle *h) > +{ > + return h->pread_initialize; > +} > + > int > nbd_unlocked_set_strict_mode (struct nbd_handle *h, uint32_t flags) > { > diff --git a/python/t/110-defaults.py b/python/t/110-defaults.py > index fb961cfd..a4262dae 100644 > --- a/python/t/110-defaults.py > +++ b/python/t/110-defaults.py > @@ -1,5 +1,5 @@ > # libnbd Python bindings > -# Copyright (C) 2010-2020 Red Hat Inc. > +# Copyright (C) 2010-2022 Red Hat Inc. > # > # This program is free software; you can redistribute it and/or modify > # it under the terms of the GNU General Public License as published by > @@ -22,5 +22,6 @@ assert h.get_export_name() == "" > assert h.get_full_info() is False > assert h.get_tls() == nbd.TLS_DISABLE > assert h.get_request_structured_replies() is True > +assert h.get_pread_initialize() is True > assert h.get_handshake_flags() == nbd.HANDSHAKE_FLAG_MASK > assert h.get_opt_mode() is False > diff --git a/python/t/120-set-non-defaults.py b/python/t/120-set-non-defaults.py > index 3da0c23e..e71c6ad0 100644 > --- a/python/t/120-set-non-defaults.py > +++ b/python/t/120-set-non-defaults.py > @@ -1,5 +1,5 @@ > # libnbd Python bindings > -# Copyright (C) 2010-2020 Red Hat Inc. > +# Copyright (C) 2010-2022 Red Hat Inc. > # > # This program is free software; you can redistribute it and/or modify > # it under the terms of the GNU General Public License as published by > @@ -33,6 +33,8 @@ if h.supports_tls(): > assert h.get_tls() == nbd.TLS_ALLOW > h.set_request_structured_replies(False) > assert h.get_request_structured_replies() is False > +h.set_pread_initialize(False) > +assert h.get_pread_initialize() is False > try: > h.set_handshake_flags(nbd.HANDSHAKE_FLAG_MASK + 1) > assert False > diff --git a/ocaml/tests/test_110_defaults.ml b/ocaml/tests/test_110_defaults.ml > index b36949f0..04aa744a 100644 > --- a/ocaml/tests/test_110_defaults.ml > +++ b/ocaml/tests/test_110_defaults.ml > @@ -1,6 +1,6 @@ > (* hey emacs, this is OCaml code: -*- tuareg -*- *) > (* libnbd OCaml test case > - * Copyright (C) 2013-2020 Red Hat Inc. > + * Copyright (C) 2013-2022 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 > @@ -28,6 +28,8 @@ let > assert (tls = NBD.TLS.DISABLE); > let sr = NBD.get_request_structured_replies nbd in > assert (sr = true); > + let init = NBD.get_pread_initialize nbd in > + assert (init = true); > let flags = NBD.get_handshake_flags nbd in > assert (flags = NBD.HANDSHAKE_FLAG.mask); > let opt = NBD.get_opt_mode nbd in > diff --git a/ocaml/tests/test_120_set_non_defaults.ml b/ocaml/tests/test_120_set_non_defaults.ml > index 67928bb5..f9498076 100644 > --- a/ocaml/tests/test_120_set_non_defaults.ml > +++ b/ocaml/tests/test_120_set_non_defaults.ml > @@ -1,6 +1,6 @@ > (* hey emacs, this is OCaml code: -*- tuareg -*- *) > (* libnbd OCaml test case > - * Copyright (C) 2013-2020 Red Hat Inc. > + * Copyright (C) 2013-2022 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 > @@ -42,6 +42,9 @@ let > NBD.set_request_structured_replies nbd false; > let sr = NBD.get_request_structured_replies nbd in > assert (sr = false); > + NBD.set_pread_initialize nbd false; > + let init = NBD.get_pread_initialize nbd in > + assert (init = false); > (try > NBD.set_handshake_flags nbd [ NBD.HANDSHAKE_FLAG.UNKNOWN 2 ]; > assert false > diff --git a/tests/errors.c b/tests/errors.c > index f597b7ee..0298da88 100644 > --- a/tests/errors.c > +++ b/tests/errors.c > @@ -213,7 +213,15 @@ main (int argc, char *argv[]) > } > > > - /* Issue a connected command when not connected. */ > + /* Issue a connected command when not connected. pread_initialize defaults > + * to set. > + */ > + if (nbd_get_pread_initialize (nbd) != 1) { > + fprintf (stderr, "%s: test failed: " > + "nbd_get_pread_initialize gave unexpected result\n", > + argv[0]); > + exit (EXIT_FAILURE); > + } > buf[0] = '1'; > if (nbd_pread (nbd, buf, 512, 0, 0) != -1) { > fprintf (stderr, "%s: test failed: " > @@ -294,7 +302,14 @@ main (int argc, char *argv[]) > } > check (EINVAL, "nbd_aio_command_completed: "); > > - /* Read from an invalid offset, client-side */ > + /* Read from an invalid offset, client-side. When pread_initialize is off, > + * libnbd should not have touched our buffer. > + */ > + if (nbd_set_pread_initialize (nbd, false) == -1) { > + fprintf (stderr, "%s\n", nbd_get_error ()); > + exit (EXIT_FAILURE); > + } > + buf[0] = '1'; > strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_BOUNDS; > if (nbd_set_strict_mode (nbd, strict) == -1) { > fprintf (stderr, "%s\n", nbd_get_error ()); > @@ -307,6 +322,12 @@ main (int argc, char *argv[]) > exit (EXIT_FAILURE); > } > check (EINVAL, "nbd_aio_pread: "); > + if (buf[0] != '1') { > + fprintf (stderr, "%s: test failed: " > + "nbd_pread incorrectly sanitized buffer on client-side error\n", > + argv[0]); > + exit (EXIT_FAILURE); > + } > > /* We guarantee callbacks will be freed even on all error paths. */ > if (nbd_aio_pread_structured (nbd, buf, 512, -1, > diff --git a/golang/libnbd_110_defaults_test.go b/golang/libnbd_110_defaults_test.go > index b3ceb45d..ca7c1c41 100644 > --- a/golang/libnbd_110_defaults_test.go > +++ b/golang/libnbd_110_defaults_test.go > @@ -1,5 +1,5 @@ > /* libnbd golang tests > - * Copyright (C) 2013-2021 Red Hat Inc. > + * Copyright (C) 2013-2022 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 > @@ -59,6 +59,14 @@ func Test110Defaults(t *testing.T) { > t.Fatalf("unexpected structured replies state") > } > > + init, err := h.GetPreadInitialize() > + if err != nil { > + t.Fatalf("could not get pread initialize state: %s", err) > + } > + if init != true { > + t.Fatalf("unexpected pread initialize state") > + } > + > flags, err := h.GetHandshakeFlags() > if err != nil { > t.Fatalf("could not get handshake flags: %s", err) > diff --git a/golang/libnbd_120_set_non_defaults_test.go b/golang/libnbd_120_set_non_defaults_test.go > index f112456c..029f0db3 100644 > --- a/golang/libnbd_120_set_non_defaults_test.go > +++ b/golang/libnbd_120_set_non_defaults_test.go > @@ -93,6 +93,18 @@ func Test120SetNonDefaults(t *testing.T) { > t.Fatalf("unexpected structured replies state") > } > > + err = h.SetPreadInitialize(false) > + if err != nil { > + t.Fatalf("could not set pread initialize state: %s", err) > + } > + init, err := h.GetPreadInitialize() > + if err != nil { > + t.Fatalf("could not get pread initialize state: %s", err) > + } > + if init != false { > + t.Fatalf("unexpected pread initialize state") > + } > + > err = h.SetHandshakeFlags(HANDSHAKE_FLAG_MASK + 1) > if err == nil { > t.Fatalf("expect failure for out-of-range flags") >Wow, this is a *lot* of work for an API that most likely no libnbd client application will use. After all -- are *we* going to be bold enough to disable pre-init? It's one thing to make an audit once and then disable pre-init, but that's a "standing promise" then, and if we miss an error check anywhere *later on*, and it turns into a security issue, someone will feel very uncomfortable. Because, "just forgetting an error check" is different from "forgetting an error check despite having promised we'd never make that mistake". Now that we have pre-init built-in and even enabled by default, personally I don't see myself ever disabling it, performance be damned. (But even wrt. performance, our comment in nbd_internal_command_common() says: "no measurable overhead".) I understand this patch may exist mainly so that no user complain about uselessly wasted cycles... I just hope we'll never actually use this API, beyond the unit tests. :) Acked-by: Laszlo Ersek <lersek at redhat.com> Thanks Laszlo