[PATCH nbdkit] tls-fallback: Fix filter for new .block_size callback
Ignore the previous patch (I think). This change works better. The filter is still kind of hairy, although I think I understand now the reasons why it is so! Rich.
This filter doesn't call the next_open function in the non-TLS case, and therefore it never opens the plugin. This leaves the internal state of nbdkit a bit strange. There is no plugin context allocated, and the last filter in the chain has a context c_next pointer of NULL. This works, provided we intercept every possible callback, check the non-TLS case, and prevent it from calling the next function (because it would dereference the NULL c_next). To avoid a crash in backend_block_size we must therefore provide a .block_size callback in this filter. --- filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/filters/tls-fallback/tls-fallback.c b/filters/tls-fallback/tls-fallback.c index fab9e58b..b34e0431 100644 --- a/filters/tls-fallback/tls-fallback.c +++ b/filters/tls-fallback/tls-fallback.c @@ -138,6 +138,20 @@ tls_fallback_get_size (nbdkit_next *next, return next->get_size (next); } +static int +tls_fallback_block_size (nbdkit_next *next, + void *handle, + uint32_t *minimum, + uint32_t *preferred, + uint32_t *maximum) +{ + if (NOT_TLS) { + *minimum = *preferred = *maximum = 0; + return 0; + } + return next->block_size (next, minimum, preferred, maximum); +} + static int tls_fallback_can_write (nbdkit_next *next, void *handle) @@ -215,6 +229,7 @@ static struct nbdkit_filter filter = { .open = tls_fallback_open, .export_description = tls_fallback_export_description, .get_size = tls_fallback_get_size, + .block_size = tls_fallback_block_size, .can_write = tls_fallback_can_write, .can_flush = tls_fallback_can_flush, .is_rotational = tls_fallback_is_rotational, -- 2.35.1
On 02/16/22 22:41, Richard W.M. Jones wrote:
This filter doesn't call the next_open function in the non-TLS case, and therefore it never opens the plugin. This leaves the internal state of nbdkit a bit strange. There is no plugin context allocated, and the last filter in the chain has a context c_next pointer of NULL.
This works, provided we intercept every possible callback, check the non-TLS case, and prevent it from calling the next function (because it would dereference the NULL c_next).
To avoid a crash in backend_block_size we must therefore provide a .block_size callback in this filter. --- filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/filters/tls-fallback/tls-fallback.c b/filters/tls-fallback/tls-fallback.c index fab9e58b..b34e0431 100644 --- a/filters/tls-fallback/tls-fallback.c +++ b/filters/tls-fallback/tls-fallback.c @@ -138,6 +138,20 @@ tls_fallback_get_size (nbdkit_next *next, return next->get_size (next); }
+static int +tls_fallback_block_size (nbdkit_next *next, + void *handle, + uint32_t *minimum, + uint32_t *preferred, + uint32_t *maximum) +{ + if (NOT_TLS) { + *minimum = *preferred = *maximum = 0; + return 0; + } + return next->block_size (next, minimum, preferred, maximum); +} + static int tls_fallback_can_write (nbdkit_next *next, void *handle) @@ -215,6 +229,7 @@ static struct nbdkit_filter filter = { .open = tls_fallback_open, .export_description = tls_fallback_export_description, .get_size = tls_fallback_get_size, + .block_size = tls_fallback_block_size, .can_write = tls_fallback_can_write, .can_flush = tls_fallback_can_flush, .is_rotational = tls_fallback_is_rotational,
This depends on the series [Libguestfs] [PATCH nbdkit 0/6] UNFINISHED Advertise block size constraints right? (I thought I'd skip reviewing that series until the dust settles on the design, between you and Eric). Thanks Laszlo
On Thu, Feb 17, 2022 at 02:20:28PM +0100, Laszlo Ersek wrote:
On 02/16/22 22:41, Richard W.M. Jones wrote:
This filter doesn't call the next_open function in the non-TLS case, and therefore it never opens the plugin. This leaves the internal state of nbdkit a bit strange. There is no plugin context allocated, and the last filter in the chain has a context c_next pointer of NULL.
This works, provided we intercept every possible callback, check the non-TLS case, and prevent it from calling the next function (because it would dereference the NULL c_next).
To avoid a crash in backend_block_size we must therefore provide a .block_size callback in this filter. --- filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/filters/tls-fallback/tls-fallback.c b/filters/tls-fallback/tls-fallback.c index fab9e58b..b34e0431 100644 --- a/filters/tls-fallback/tls-fallback.c +++ b/filters/tls-fallback/tls-fallback.c @@ -138,6 +138,20 @@ tls_fallback_get_size (nbdkit_next *next, return next->get_size (next); }
+static int +tls_fallback_block_size (nbdkit_next *next, + void *handle, + uint32_t *minimum, + uint32_t *preferred, + uint32_t *maximum) +{ + if (NOT_TLS) { + *minimum = *preferred = *maximum = 0; + return 0; + } + return next->block_size (next, minimum, preferred, maximum); +} + static int tls_fallback_can_write (nbdkit_next *next, void *handle) @@ -215,6 +229,7 @@ static struct nbdkit_filter filter = { .open = tls_fallback_open, .export_description = tls_fallback_export_description, .get_size = tls_fallback_get_size, + .block_size = tls_fallback_block_size, .can_write = tls_fallback_can_write, .can_flush = tls_fallback_can_flush, .is_rotational = tls_fallback_is_rotational,
This depends on the series
[Libguestfs] [PATCH nbdkit 0/6] UNFINISHED Advertise block size constraints
right? (I thought I'd skip reviewing that series until the dust settles on the design, between you and Eric).
Yes, that's right. I could not make this filter fail except when used with the block size constraints series. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Wed, Feb 16, 2022 at 09:41:17PM +0000, Richard W.M. Jones wrote:
Ignore the previous patch (I think). This change works better.
The filter is still kind of hairy, although I think I understand now the reasons why it is so!
Yep, CVE-2021-3716 mandates some of the complexity. I should at least push a patch adding more comments (including the CVE number) in tls-fallback.c, rather than assuming you can piece it together from the blurb in nbdkit-security.pod. And yes, this version is much better than your v1 (which would have reintroduced the CVE that this filter was designed to prevent). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones