Re: [Libguestfs] [libnbd PATCH v2 6/6] info: Tolerate missing size
On Thu, Sep 21, 2023 at 03:58:05PM -0500, Eric Blake wrote:
> As previous patches showed, the NBD spec does not yet forbid a server
> sending us a size that does not fit in int64_t. We should gracefully
> handle this during nbdinfo, rather than giving up early.
>
> With the same one-line hack to qemu to set the most significant bit of
> the export size, output changes from pre-patch:
>
> $ ./run nbdinfo nbd://localhost
> /home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
> 9223372036854781440 which does not fit in signed result: Value too large for
> defined data type
> qemu-nbd: option negotiation failed: Failed to read opts magic: Unexpected
> end-of-file before all data were read
>
> to post-patch:
>
> $ ./run nbdinfo nbd://localhost
> protocol: newstyle-fixed without TLS, using extended packets
> ...
> block_size_maximum: 33554432
>
> or
>
> $ ./run nbdinfo nbd://localhost --json
> {
> "protocol": "newstyle-fixed",
> ...
> "block_size_maximum": 33554432,
> "export-size-str": "unavailable"
> } ]
> }
>
> Sadly, since writing a server with such large export sizes requires a
> one-off hack, I don't see the point in adding a unit test.
>
> Signed-off-by: Eric Blake
> ---
> info/show.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/info/show.c b/info/show.c
> index a71d837e..3d80545e 100644
> --- a/info/show.c
> +++ b/info/show.c
> @@ -46,7 +46,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
> bool first, bool last)
> {
>int64_t i, size;
> - char size_str[HUMAN_SIZE_LONGEST];
> + char size_str[HUMAN_SIZE_LONGEST] = "unavailable";
>bool human_size_flag;
>char *export_name = NULL;
>char *export_desc = NULL;
> @@ -89,13 +89,10 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
> return false;
>}
>size = nbd_get_size (nbd);
> - if (size == -1) {
> -fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> -exit (EXIT_FAILURE);
> + if (size >= 0) {
> +human_size (size_str, size, _size_flag);
>}
>
> - human_size (size_str, size, _size_flag);
> -
>if (uri_is_meaningful ())
> uri = nbd_get_uri (nbd);
>
> @@ -130,7 +127,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
> show_context = true;
>
>/* Get content last, as it moves the connection out of negotiating */
> - content = get_content (nbd, size);
> + if (size >= 0)
> +content = get_content (nbd, size);
>
>if (!json_output) {
> ansi_colour (ANSI_FG_BOLD_BLACK, fp);
> @@ -140,10 +138,12 @@ show_one_export (struct nbd_handle *nbd, const char
> *desc,
> fprintf (fp, ":\n");
> if (desc && *desc)
>fprintf (fp, "\tdescription: %s\n", desc);
> -if (human_size_flag)
> - fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
> -else
> - fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
> +if (size >= 0) {
> + if (human_size_flag)
> +fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
> + else
> +fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
> +}
> if (content)
>fprintf (fp, "\tcontent: %s\n", content);
> if (uri)
> @@ -273,7 +273,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
> block_maximum);
>
> /* Put this one at the end because of the stupid comma thing in JSON. */
> -fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
> +if (size >= 0)
> + fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
> fprintf (fp, "\t\"export-size-str\": \"%s\"\n", size_str);
>
> if (last)
Reviewed-by: Richard W.M. Jones
--
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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 4/6] block_status: Fix assertion with large server size
On Thu, Sep 21, 2023 at 03:58:03PM -0500, Eric Blake wrote:
> As mentioned in the previous commit ("api: Sanitize sizes larger than
> INT64_MAX"), the NBD spec does not (yet) prohibit a server from
> advertising a size larger than INT64_MAX. While we can't report such
> size to the user, v1.16 was at least internally consistent with the
> server's size everywhere else.
>
> But when adding code to implement 64-bit block status, I intentionally
> wanted to guarantee that the callback sees a positive int64_t length
> even when the server's wire value can be 64 bits, for ease in writing
> language bindings (OCaml in particular benefitted from that
> guarantee), even though I didn't document that in the API until now.
> That was because I had blindly assumed that the server's exportsize
> fit in 63 bits, and therefore I didn't have to worry about arithmetic
> overflow once I capped the extent length to exportsize. But the
> fuzzer quickly proved me wrong. What's more, with the same one-line
> hack to qemu as shown in the previous commit to advertise a size with
> the high-order bit set,
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> > def f(*k):
> > pass
> > h.block_status(1,0,f)
> > EOF
> nbdsh: generator/states-reply-chunk.c:554:
> enter_STATE_REPLY_CHUNK_REPLY_RECV_BS_ENTRIES: Assertion `h->exportsize <=
> INT64_MAX' failed.
> Aborted (core dumped)
>
> even though it did not dump core in v1.16.
>
> Since my assumption was bad, rewrite the logic to increment total
> after bounds-checking rather than before, and to bounds-check based on
> INT64_MAX+1-64M rather than on the export size. As before, we never
> report a zero-length extent to the callback. Whether or not secalert
> advises us to create a CVE for the previous patch, this bug does not
> deserve its own CVE as it was only introduced in recent unstable
> releases.
>
> Fixes: e8d837d306 ("block_status: Add some sanity checking of server
> lengths", v1.17.4)
> Thanks: Richard W.M. Jones
> Signed-off-by: Eric Blake
> ---
> generator/states-reply-chunk.c | 49 ++
> generator/C.ml | 2 +-
> 2 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
> index 2cebe456..20407d91 100644
> --- a/generator/states-reply-chunk.c
> +++ b/generator/states-reply-chunk.c
> @@ -547,11 +547,16 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
>break;
> }
>
> +/* Be careful to avoid arithmetic overflow, even when the user
> + * disabled LIBNBD_STRICT_BOUNDS to pass a suspect offset, or the
> + * server returns suspect lengths or advertised exportsize larger
> + * than 63 bits. We guarantee that callbacks will not see a
> + * length exceeding INT64_MAX or the advertised h->exportsize.
> + */
> name = h->meta_contexts.ptr[i].name;
> -total = 0;
> -cap = h->exportsize - cmd->offset;
> -assert (cap <= h->exportsize);
> -assert (h->exportsize <= INT64_MAX);
> +total = cap = 0;
> +if (cmd->offset <= h->exportsize)
> + cap = h->exportsize - cmd->offset;
>
> /* Need to byte-swap the entries returned into the callback size
> * requested by the caller. The NBD protocol allows truncation as
> @@ -560,10 +565,11 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
> * don't like. We stop iterating on a zero-length extent (error
> * only if it is the first extent), on an extent beyond the
> * exportsize (unconditional error after truncating to
> - * exportsize), and on an extent exceeding a 32-bit callback (no
> - * error, and to simplify alignment, we truncate to 4G-64M); but
> - * do not diagnose issues with the server's length alignments,
> - * flag values, nor compliance with the REQ_ONE command flag.
> + * exportsize), and on an extent exceeding a callback length limit
> + * (no error, and to simplify alignment, we truncate to 64M before
> + * the limit); but we do not diagnose issues with the server's
> + * length alignments, flag values, nor compliance with the REQ_ONE
> + * command flag.
> */
> for (i = 0, stop = false; i < h->bs_count && !stop; ++i) {
>if (type == NBD_REPLY_TYPE_BLOCK_STATUS) {
> @@ -572,16 +578,12 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
>}
>else {
> orig_len = len = be64toh (h->bs_raw.wide[i].length);
> -if (len > h->exportsize) {
> - /* Since we already asserted exportsize is at most 63 bits,
> - * this ensures the extent length will appear positive even
> - * if treated as signed; treat this as an error now, rather
> - * than waiting for the comparison to cap later, to avoid
> - * arithmetic overflow.
> +if (len > INT64_MAX) {
> + /* Pick an aligned value rather than overflowing 64-bit
> + * callback; this does not require an error.
> */
>stop =
Re: [Libguestfs] [libnbd PATCH v2 5/6] block_status: Fix assertion on bad 64-bit block status reply
On Thu, Sep 21, 2023 at 03:58:04PM -0500, Eric Blake wrote:
> If a server replies to a block status command with an invalid count in
> NBD_REPLY_TYPE_BLOCK_STATUS_EXT, we correctly detect the server's
> error, but fail to mark that we've consumed enough data off the wire
> to resync back to the server's next reply. Rich's fuzzing run
> initially found this, but I was able to quickly write a one-byte patch
> on top of my pending qemu patches [1] to reproduce it:
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg05231.html
>
> | diff --git i/nbd/server.c w/nbd/server.c
> | index 898580a9b0b..bd8d46ba3c4 100644
> | --- i/nbd/server.c
> | +++ w/nbd/server.c
> | @@ -2326,7 +2326,7 @@ nbd_co_send_extents(NBDClient *client, NBDRequest
> *request, NBDExtentArray *ea,
> | iov[1].iov_base = _ext;
> | iov[1].iov_len = sizeof(meta_ext);
> | stl_be_p(_ext.context_id, context_id);
> | -stl_be_p(_ext.count, ea->count);
> | +stl_be_p(_ext.count, !ea->count);
> |
> | nbd_extent_array_convert_to_be(ea);
> | iov[2].iov_base = ea->extents;
>
> then with a just-built 'qemu-nbd -f raw -r file -t &' running, we have
> pre-patch:
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> > def f(*k):
> > pass
> > try:
> > h.block_status(1,0,f)
> > except nbd.Error as ex:
> > print(ex.string)
> > h.shutdown()
> > EOF
> nbdsh: generator/states-reply-chunk.c:701:
> enter_STATE_REPLY_CHUNK_REPLY_FINISH: Assertion `h->payload_left == 0' failed.
> Aborted (core dumped)
>
> vs. post-patch:
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> > def f(*k):
> > pass
> > try:
> > h.block_status(1,0,f)
> > except nbd.Error as ex:
> > print(ex.string)
> > h.shutdown()
> > EOF
> nbd_block_status: block-status: command failed: Protocol error
>
> Appears to be a casualty of rebasing: I added h->payload_left
> verification fairly late in the game, then floated it earlier in the
> series, and missed a spot where I added a state machine jump to RESYNC
> without having updated h->payload_left. An audit of h->hlen
> modification sites show that all other chunked reads updated
> h->payload_left appropriately (often in the next statement, but
> sometimes in a later state when that made logic easier).
>
> Requires a non-compliant server, and only possible when extended
> headers are negotiated, which does not affect any stable released
> libnbd. Thus, there is no reason to create a CVE, although since I
> will already be doing a security info email about previous patches
> also addressing fuzzer findings, I can mention this at the same time.
>
> Fixes: ab992766cd ("block_status: Accept 64-bit extents during block status")
> Thanks: Richard W.M. Jones
> Signed-off-by: Eric Blake
> ---
> generator/states-reply-chunk.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
> index 20407d91..5a31c192 100644
> --- a/generator/states-reply-chunk.c
> +++ b/generator/states-reply-chunk.c
> @@ -476,6 +476,7 @@ REPLY.CHUNK_REPLY.RECV_BS_HEADER:
>if (h->bs_count != be32toh (h->sbuf.reply.payload.bs_hdr_64.count)) {
> h->rbuf = NULL;
> h->rlen = h->payload_left;
> +h->payload_left = 0;
> SET_NEXT_STATE (%RESYNC);
> return 0;
>}
Reviewed-by: Richard W.M. Jones
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 2/6] fuzzing: Disable client-side strictness checks
On Thu, Sep 21, 2023 at 03:58:01PM -0500, Eric Blake wrote: > When fuzzing, it is more desirable to always provoke the server into > sending a response, rather than sometimes accidentally skipping a wire > call because a client-side strictness test failed. > > [Our fuzzer could probably be made even more powerful by changing the > fuzzer input file to be a series of records, where each record is the > API to call and then the server's response; right now, the sequence of > APIs called is hard-coded, which is not as powerful at testing > potential cross-command coupling. But that's a project for another > day.] Intrigued by how this would work exactly. The current file is just a stream of bytes from the "server" (the output from libnbd is discarded). Would the input file be of the form: ( [ request + args ] [ stream of bytes from server ] )* where [ request + args ] would be a simple serialized representation of a synchronous nbd_* API call? I think that would work ... Rich. > Signed-off-by: Eric Blake > Reviewed-by: Richard W.M. Jones > Reviewed-by: Laszlo Ersek > --- > fuzzing/libnbd-fuzz-wrapper.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fuzzing/libnbd-fuzz-wrapper.c b/fuzzing/libnbd-fuzz-wrapper.c > index cbd55380..fcd1d04c 100644 > --- a/fuzzing/libnbd-fuzz-wrapper.c > +++ b/fuzzing/libnbd-fuzz-wrapper.c > @@ -193,8 +193,11 @@ client (int sock) >} > >/* Note we ignore errors in these calls because we are only > - * interested in whether the process crashes. > + * interested in whether the process crashes. Likewise, we don't > + * want to accidentally avoid sending traffic to the server merely > + * because client side strictness sees a problem. > */ > + nbd_set_strict_mode (nbd, 0); > >/* Enable a metadata context, for block status below. */ >nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION); > -- > 2.41.0 -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 3/6] api: Sanitize sizes larger than INT64_MAX
On Thu, Sep 21, 2023 at 03:58:02PM -0500, Eric Blake wrote:
> Our stable API has always claimed that nbd_get_size() reports a
> non-negative value on success, and -1 on failure. While we know of no
> existing production server (such as nbdkit, qemu-nbd, nbd-server) that
> would advertise a size larger than off_t, the NBD spec has not yet
> committed to enforcing a hard maximum of an export size as a signed
> integer; at present, the protocol mandates merely:
> S: 64 bits, size of the export in bytes (unsigned)
>
> I've considered proposing a spec patch to upstream NBD to require a
> positive signed value, and can point to this patch to help justify
> such a patch - but that hasn't happened yet. Thus, it should be no
> surprise that our fuzzer was quickly able to emulate a theoretical
> server that claims a size larger than INT64_MAX - at which point,
> nbd_get_size() is returning a negative value that is not -1, and the
> API documentation was unclear whether the application should treat
> this as success or failure. However, as we did not crash, the fuzzer
> did not flag it as interesting in v1.16.
>
> I considered changing nbd_internal_set_size_and_flags() to move the
> state machine to DEAD for any server advertising something so large
> (that is, nbd_connect_*() would be unable to connect to such a
> server); but without the NBD spec mandating such a limit, that is an
> artificial constraint. Instead, this patch chooses to normalize all
> wire values that can't fit in the int64_t return type to an EOVERFLOW
> error, and clarifies the API documentation accordingly. Existing
> clients that depend on a known positive size and check for
> nbd_get_size()<0 are not impacted, while those that check for ==-1
> will now reject such uncommon servers instead of potentially using a
> negative value in subsequent computations (the latter includes
> nbdinfo, which changes from reporting a negative size to instead
> printing an error message). Meanwhile, clients that never called
> nbd_get_size() (presumably because they infer the size from other
> means, or because they intend to access offsets above 2^63 despite not
> being able to reliably see that size from nbd_get_size) can still do
> so.
>
> Next, I audited all instances of 'exportsize', to see if libnbd itself
> has any arithmetic issues when a large size is stored. My results for
> v1.16 are as follows:
>
> lib/nbd-protocol.h and lib/internal.h both have uint64_t exportsize
> (so we are already treating size as unsigned both for wire and
> internal representation - no nasty surprises with sign extension).
>
> generator/states-{oldstyle,opt-{go,export-name}}.c grab exportsize off
> the wire, byteswap if needed, and pass it to
> nbd_internal_set_size_and_flags() without further inspection.
>
> lib/flags.c has nbd_internal_set_size_and_flags() which stores the
> wire value into the internal value, then nbd_unlocked_get_size() which
> reports the wire value as-is (prior to this patch adding EOVERFLOW
> normalization). nbd_internal_set_block_size() mentions exportsize in
> comments, but does not actually compare against it.
>
> lib/rw.c added LIBNBD_STRICT_BOUNDS checking in v1.6 via:
> if ((h->strict & LIBNBD_STRICT_BOUNDS) &&
> (offset > h->exportsize || count > h->exportsize - offset)) {
> where offset and count are also unsigned values (count is 32-bit in
> 1.16, 64-bit in master); but the order of comparisons is robust
> against wraparound when using unsigned math. Earlier releases, or
> clients which use nbd_set_strict_mode(,0) to skip bounds checking,
> have been assuming the server will reject requests with a weird
> offset (most servers do, although the NBD spec only recommends
> that sever behavior, by stating that the burden is on the client
> to pass in-bounds requests in the first place).
>
> generator/states-reply-chunk.c added comparisons against exportsize
> for NBD_OPT_BLOCK_STATUS only after 1.16, and that's the subject of
> the next patch.
>
> No other direct uses of exportsize exist, so libnbd itself can
> internally handle sizes larger than 2^63 without misbehaving, outside
> of nbd_get_size(), even if such an offset was computed from taking the
> negative int64_t value of nbd_get_size() and turning it back into
> uint64_t offset parameter.
>
> I also audited our other APIs - everything else uses a parameter of
> type UInt64 in the generator for offsets, which is translated in C.ml
> to uint64_t; and we already know we are safe when C converts negative
> int64_t values into uint64_t.
>
> For other languages, Python.ml generates code for UInt64 by using
> 'PyArg_ParseValue("K")' with a detour through 'unsigned long long' in
> case 'uint64_t' is a different type rank despite being the same number
> of bits. The Python documentation states that "K" converts an
> arbitrary python integer value to a C uint64_t without overflow
> checking - so it is already possible to pass offset values larger than
> 2^63 in
[Libguestfs] [libnbd PATCH v2 3/6] api: Sanitize sizes larger than INT64_MAX
Our stable API has always claimed that nbd_get_size() reports a
non-negative value on success, and -1 on failure. While we know of no
existing production server (such as nbdkit, qemu-nbd, nbd-server) that
would advertise a size larger than off_t, the NBD spec has not yet
committed to enforcing a hard maximum of an export size as a signed
integer; at present, the protocol mandates merely:
S: 64 bits, size of the export in bytes (unsigned)
I've considered proposing a spec patch to upstream NBD to require a
positive signed value, and can point to this patch to help justify
such a patch - but that hasn't happened yet. Thus, it should be no
surprise that our fuzzer was quickly able to emulate a theoretical
server that claims a size larger than INT64_MAX - at which point,
nbd_get_size() is returning a negative value that is not -1, and the
API documentation was unclear whether the application should treat
this as success or failure. However, as we did not crash, the fuzzer
did not flag it as interesting in v1.16.
I considered changing nbd_internal_set_size_and_flags() to move the
state machine to DEAD for any server advertising something so large
(that is, nbd_connect_*() would be unable to connect to such a
server); but without the NBD spec mandating such a limit, that is an
artificial constraint. Instead, this patch chooses to normalize all
wire values that can't fit in the int64_t return type to an EOVERFLOW
error, and clarifies the API documentation accordingly. Existing
clients that depend on a known positive size and check for
nbd_get_size()<0 are not impacted, while those that check for ==-1
will now reject such uncommon servers instead of potentially using a
negative value in subsequent computations (the latter includes
nbdinfo, which changes from reporting a negative size to instead
printing an error message). Meanwhile, clients that never called
nbd_get_size() (presumably because they infer the size from other
means, or because they intend to access offsets above 2^63 despite not
being able to reliably see that size from nbd_get_size) can still do
so.
Next, I audited all instances of 'exportsize', to see if libnbd itself
has any arithmetic issues when a large size is stored. My results for
v1.16 are as follows:
lib/nbd-protocol.h and lib/internal.h both have uint64_t exportsize
(so we are already treating size as unsigned both for wire and
internal representation - no nasty surprises with sign extension).
generator/states-{oldstyle,opt-{go,export-name}}.c grab exportsize off
the wire, byteswap if needed, and pass it to
nbd_internal_set_size_and_flags() without further inspection.
lib/flags.c has nbd_internal_set_size_and_flags() which stores the
wire value into the internal value, then nbd_unlocked_get_size() which
reports the wire value as-is (prior to this patch adding EOVERFLOW
normalization). nbd_internal_set_block_size() mentions exportsize in
comments, but does not actually compare against it.
lib/rw.c added LIBNBD_STRICT_BOUNDS checking in v1.6 via:
if ((h->strict & LIBNBD_STRICT_BOUNDS) &&
(offset > h->exportsize || count > h->exportsize - offset)) {
where offset and count are also unsigned values (count is 32-bit in
1.16, 64-bit in master); but the order of comparisons is robust
against wraparound when using unsigned math. Earlier releases, or
clients which use nbd_set_strict_mode(,0) to skip bounds checking,
have been assuming the server will reject requests with a weird
offset (most servers do, although the NBD spec only recommends
that sever behavior, by stating that the burden is on the client
to pass in-bounds requests in the first place).
generator/states-reply-chunk.c added comparisons against exportsize
for NBD_OPT_BLOCK_STATUS only after 1.16, and that's the subject of
the next patch.
No other direct uses of exportsize exist, so libnbd itself can
internally handle sizes larger than 2^63 without misbehaving, outside
of nbd_get_size(), even if such an offset was computed from taking the
negative int64_t value of nbd_get_size() and turning it back into
uint64_t offset parameter.
I also audited our other APIs - everything else uses a parameter of
type UInt64 in the generator for offsets, which is translated in C.ml
to uint64_t; and we already know we are safe when C converts negative
int64_t values into uint64_t.
For other languages, Python.ml generates code for UInt64 by using
'PyArg_ParseValue("K")' with a detour through 'unsigned long long' in
case 'uint64_t' is a different type rank despite being the same number
of bits. The Python documentation states that "K" converts an
arbitrary python integer value to a C uint64_t without overflow
checking - so it is already possible to pass offset values larger than
2^63 in nbdsh; while values larger than 2^64 or negative values are
effectively truncated as if with modulo math. Enhancing the language
bindings to explicitly detect over/underflow is outside the scope of
this patch (and could surprise users who
[Libguestfs] [libnbd PATCH v2 0/6] Fix fuzzer fallout
Cat's out of the bag: Rich's fuzzer run found not one, but two independent assertion failures that a malicious server could trigger in my recent 64-bit extension code additions. What's more, in the process of fixing them, we've discovered another long-standing issue where nbd_get_size() returns confusing results compared to its documentation, when talking to an odd server that reports a really large export size. After off-list discussion between Rich, Laszlo, and myself, we didn't think an embargoed CVE against libnbd is necessary (the assertion failures only happen to unstable releases, and the nbd_get_size() misbehavior does not happen with normal servers and has been in place since v1.0, so it is nothing new), so I am posting the series now for public review. But we will still be reaching out to secalert for their opinion (it may be that they still see a low-priority exploit in an app that gets confused when trying to use a negative size as a loop bound, for example). Once they answer, and regardless of whether we end up doing a libnbd CVE after all, I will follow up to the mailing list with a security incident (client apps that demand a positive export size should probably favor nbd_get_size()<0 over nbd_get_size()==-1). Eric Blake (6): states: Tweak comment in OPT_GO state handler fuzzing: Disable client-side strictness checks api: Sanitize sizes larger than INT64_MAX block_status: Fix assertion with large server size block_status: Fix assertion on bad 64-bit block status reply info: Tolerate missing size generator/API.ml | 6 +++- generator/states-newstyle-opt-go.c | 5 ++- generator/states-reply-chunk.c | 50 -- generator/C.ml | 2 +- lib/flags.c| 6 fuzzing/libnbd-fuzz-wrapper.c | 5 ++- info/show.c| 25 --- 7 files changed, 60 insertions(+), 39 deletions(-) -- 2.41.0 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH v2 5/6] block_status: Fix assertion on bad 64-bit block status reply
If a server replies to a block status command with an invalid count in
NBD_REPLY_TYPE_BLOCK_STATUS_EXT, we correctly detect the server's
error, but fail to mark that we've consumed enough data off the wire
to resync back to the server's next reply. Rich's fuzzing run
initially found this, but I was able to quickly write a one-byte patch
on top of my pending qemu patches [1] to reproduce it:
[1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg05231.html
| diff --git i/nbd/server.c w/nbd/server.c
| index 898580a9b0b..bd8d46ba3c4 100644
| --- i/nbd/server.c
| +++ w/nbd/server.c
| @@ -2326,7 +2326,7 @@ nbd_co_send_extents(NBDClient *client, NBDRequest
*request, NBDExtentArray *ea,
| iov[1].iov_base = _ext;
| iov[1].iov_len = sizeof(meta_ext);
| stl_be_p(_ext.context_id, context_id);
| -stl_be_p(_ext.count, ea->count);
| +stl_be_p(_ext.count, !ea->count);
|
| nbd_extent_array_convert_to_be(ea);
| iov[2].iov_base = ea->extents;
then with a just-built 'qemu-nbd -f raw -r file -t &' running, we have
pre-patch:
$ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> def f(*k):
> pass
> try:
> h.block_status(1,0,f)
> except nbd.Error as ex:
> print(ex.string)
> h.shutdown()
> EOF
nbdsh: generator/states-reply-chunk.c:701:
enter_STATE_REPLY_CHUNK_REPLY_FINISH: Assertion `h->payload_left == 0' failed.
Aborted (core dumped)
vs. post-patch:
$ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> def f(*k):
> pass
> try:
> h.block_status(1,0,f)
> except nbd.Error as ex:
> print(ex.string)
> h.shutdown()
> EOF
nbd_block_status: block-status: command failed: Protocol error
Appears to be a casualty of rebasing: I added h->payload_left
verification fairly late in the game, then floated it earlier in the
series, and missed a spot where I added a state machine jump to RESYNC
without having updated h->payload_left. An audit of h->hlen
modification sites show that all other chunked reads updated
h->payload_left appropriately (often in the next statement, but
sometimes in a later state when that made logic easier).
Requires a non-compliant server, and only possible when extended
headers are negotiated, which does not affect any stable released
libnbd. Thus, there is no reason to create a CVE, although since I
will already be doing a security info email about previous patches
also addressing fuzzer findings, I can mention this at the same time.
Fixes: ab992766cd ("block_status: Accept 64-bit extents during block status")
Thanks: Richard W.M. Jones
Signed-off-by: Eric Blake
---
generator/states-reply-chunk.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
index 20407d91..5a31c192 100644
--- a/generator/states-reply-chunk.c
+++ b/generator/states-reply-chunk.c
@@ -476,6 +476,7 @@ REPLY.CHUNK_REPLY.RECV_BS_HEADER:
if (h->bs_count != be32toh (h->sbuf.reply.payload.bs_hdr_64.count)) {
h->rbuf = NULL;
h->rlen = h->payload_left;
+h->payload_left = 0;
SET_NEXT_STATE (%RESYNC);
return 0;
}
--
2.41.0
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH v2 1/6] states: Tweak comment in OPT_GO state handler
While auditing code, I stumbled across a confusing comment which
references a state name that does not exist. In addition to improving
the comment, I added an assertion, since there is action at a distance
(prepare_for_reply_payload is in states-newstyle.c) for ignoring the
oversized payload.
Signed-off-by: Eric Blake
Reviewed-by: Richard W.M. Jones
Reviewed-by: Laszlo Ersek
---
generator/states-newstyle-opt-go.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/generator/states-newstyle-opt-go.c
b/generator/states-newstyle-opt-go.c
index 2ef440d4..5bc9a9ae 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -149,8 +149,11 @@ NEWSTYLE.OPT_GO.CHECK_REPLY:
switch (reply) {
case NBD_REP_INFO:
-if (len > maxpayload /* see RECV_NEWSTYLE_OPT_GO_REPLY */)
+if (len > maxpayload) {
+ /* See prepare_for_reply_payload, used in RECV_REPLY */
+ assert (h->rbuf == NULL);
debug (h, "skipping large NBD_REP_INFO");
+}
else {
uint16_t info;
uint64_t exportsize;
--
2.41.0
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH v2 2/6] fuzzing: Disable client-side strictness checks
When fuzzing, it is more desirable to always provoke the server into sending a response, rather than sometimes accidentally skipping a wire call because a client-side strictness test failed. [Our fuzzer could probably be made even more powerful by changing the fuzzer input file to be a series of records, where each record is the API to call and then the server's response; right now, the sequence of APIs called is hard-coded, which is not as powerful at testing potential cross-command coupling. But that's a project for another day.] Signed-off-by: Eric Blake Reviewed-by: Richard W.M. Jones Reviewed-by: Laszlo Ersek --- fuzzing/libnbd-fuzz-wrapper.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fuzzing/libnbd-fuzz-wrapper.c b/fuzzing/libnbd-fuzz-wrapper.c index cbd55380..fcd1d04c 100644 --- a/fuzzing/libnbd-fuzz-wrapper.c +++ b/fuzzing/libnbd-fuzz-wrapper.c @@ -193,8 +193,11 @@ client (int sock) } /* Note we ignore errors in these calls because we are only - * interested in whether the process crashes. + * interested in whether the process crashes. Likewise, we don't + * want to accidentally avoid sending traffic to the server merely + * because client side strictness sees a problem. */ + nbd_set_strict_mode (nbd, 0); /* Enable a metadata context, for block status below. */ nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION); -- 2.41.0 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH v2 4/6] block_status: Fix assertion with large server size
As mentioned in the previous commit ("api: Sanitize sizes larger than
INT64_MAX"), the NBD spec does not (yet) prohibit a server from
advertising a size larger than INT64_MAX. While we can't report such
size to the user, v1.16 was at least internally consistent with the
server's size everywhere else.
But when adding code to implement 64-bit block status, I intentionally
wanted to guarantee that the callback sees a positive int64_t length
even when the server's wire value can be 64 bits, for ease in writing
language bindings (OCaml in particular benefitted from that
guarantee), even though I didn't document that in the API until now.
That was because I had blindly assumed that the server's exportsize
fit in 63 bits, and therefore I didn't have to worry about arithmetic
overflow once I capped the extent length to exportsize. But the
fuzzer quickly proved me wrong. What's more, with the same one-line
hack to qemu as shown in the previous commit to advertise a size with
the high-order bit set,
$ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> def f(*k):
> pass
> h.block_status(1,0,f)
> EOF
nbdsh: generator/states-reply-chunk.c:554:
enter_STATE_REPLY_CHUNK_REPLY_RECV_BS_ENTRIES: Assertion `h->exportsize <=
INT64_MAX' failed.
Aborted (core dumped)
even though it did not dump core in v1.16.
Since my assumption was bad, rewrite the logic to increment total
after bounds-checking rather than before, and to bounds-check based on
INT64_MAX+1-64M rather than on the export size. As before, we never
report a zero-length extent to the callback. Whether or not secalert
advises us to create a CVE for the previous patch, this bug does not
deserve its own CVE as it was only introduced in recent unstable
releases.
Fixes: e8d837d306 ("block_status: Add some sanity checking of server lengths",
v1.17.4)
Thanks: Richard W.M. Jones
Signed-off-by: Eric Blake
---
generator/states-reply-chunk.c | 49 ++
generator/C.ml | 2 +-
2 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
index 2cebe456..20407d91 100644
--- a/generator/states-reply-chunk.c
+++ b/generator/states-reply-chunk.c
@@ -547,11 +547,16 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
break;
}
+/* Be careful to avoid arithmetic overflow, even when the user
+ * disabled LIBNBD_STRICT_BOUNDS to pass a suspect offset, or the
+ * server returns suspect lengths or advertised exportsize larger
+ * than 63 bits. We guarantee that callbacks will not see a
+ * length exceeding INT64_MAX or the advertised h->exportsize.
+ */
name = h->meta_contexts.ptr[i].name;
-total = 0;
-cap = h->exportsize - cmd->offset;
-assert (cap <= h->exportsize);
-assert (h->exportsize <= INT64_MAX);
+total = cap = 0;
+if (cmd->offset <= h->exportsize)
+ cap = h->exportsize - cmd->offset;
/* Need to byte-swap the entries returned into the callback size
* requested by the caller. The NBD protocol allows truncation as
@@ -560,10 +565,11 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
* don't like. We stop iterating on a zero-length extent (error
* only if it is the first extent), on an extent beyond the
* exportsize (unconditional error after truncating to
- * exportsize), and on an extent exceeding a 32-bit callback (no
- * error, and to simplify alignment, we truncate to 4G-64M); but
- * do not diagnose issues with the server's length alignments,
- * flag values, nor compliance with the REQ_ONE command flag.
+ * exportsize), and on an extent exceeding a callback length limit
+ * (no error, and to simplify alignment, we truncate to 64M before
+ * the limit); but we do not diagnose issues with the server's
+ * length alignments, flag values, nor compliance with the REQ_ONE
+ * command flag.
*/
for (i = 0, stop = false; i < h->bs_count && !stop; ++i) {
if (type == NBD_REPLY_TYPE_BLOCK_STATUS) {
@@ -572,16 +578,12 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
}
else {
orig_len = len = be64toh (h->bs_raw.wide[i].length);
-if (len > h->exportsize) {
- /* Since we already asserted exportsize is at most 63 bits,
- * this ensures the extent length will appear positive even
- * if treated as signed; treat this as an error now, rather
- * than waiting for the comparison to cap later, to avoid
- * arithmetic overflow.
+if (len > INT64_MAX) {
+ /* Pick an aligned value rather than overflowing 64-bit
+ * callback; this does not require an error.
*/
stop = true;
- cmd->error = cmd->error ? : EPROTO;
- len = h->exportsize;
+ len = INT64_MAX + 1ULL - MAX_REQUEST_SIZE;
}
if (len > UINT32_MAX && !cmd->cb.wide) {
/* Pick an aligned value rather than overflowing 32-bit
[Libguestfs] [libnbd PATCH v2 6/6] info: Tolerate missing size
As previous patches showed, the NBD spec does not yet forbid a server
sending us a size that does not fit in int64_t. We should gracefully
handle this during nbdinfo, rather than giving up early.
With the same one-line hack to qemu to set the most significant bit of
the export size, output changes from pre-patch:
$ ./run nbdinfo nbd://localhost
/home/eblake/libnbd/info/.libs/nbdinfo: nbd_get_size: server claims size
9223372036854781440 which does not fit in signed result: Value too large for
defined data type
qemu-nbd: option negotiation failed: Failed to read opts magic: Unexpected
end-of-file before all data were read
to post-patch:
$ ./run nbdinfo nbd://localhost
protocol: newstyle-fixed without TLS, using extended packets
...
block_size_maximum: 33554432
or
$ ./run nbdinfo nbd://localhost --json
{
"protocol": "newstyle-fixed",
...
"block_size_maximum": 33554432,
"export-size-str": "unavailable"
} ]
}
Sadly, since writing a server with such large export sizes requires a
one-off hack, I don't see the point in adding a unit test.
Signed-off-by: Eric Blake
---
info/show.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/info/show.c b/info/show.c
index a71d837e..3d80545e 100644
--- a/info/show.c
+++ b/info/show.c
@@ -46,7 +46,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
bool first, bool last)
{
int64_t i, size;
- char size_str[HUMAN_SIZE_LONGEST];
+ char size_str[HUMAN_SIZE_LONGEST] = "unavailable";
bool human_size_flag;
char *export_name = NULL;
char *export_desc = NULL;
@@ -89,13 +89,10 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
return false;
}
size = nbd_get_size (nbd);
- if (size == -1) {
-fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
-exit (EXIT_FAILURE);
+ if (size >= 0) {
+human_size (size_str, size, _size_flag);
}
- human_size (size_str, size, _size_flag);
-
if (uri_is_meaningful ())
uri = nbd_get_uri (nbd);
@@ -130,7 +127,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
show_context = true;
/* Get content last, as it moves the connection out of negotiating */
- content = get_content (nbd, size);
+ if (size >= 0)
+content = get_content (nbd, size);
if (!json_output) {
ansi_colour (ANSI_FG_BOLD_BLACK, fp);
@@ -140,10 +138,12 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
fprintf (fp, ":\n");
if (desc && *desc)
fprintf (fp, "\tdescription: %s\n", desc);
-if (human_size_flag)
- fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
-else
- fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
+if (size >= 0) {
+ if (human_size_flag)
+fprintf (fp, "\texport-size: %" PRIi64 " (%s)\n", size, size_str);
+ else
+fprintf (fp, "\texport-size: %" PRIi64 "\n", size);
+}
if (content)
fprintf (fp, "\tcontent: %s\n", content);
if (uri)
@@ -273,7 +273,8 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
block_maximum);
/* Put this one at the end because of the stupid comma thing in JSON. */
-fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
+if (size >= 0)
+ fprintf (fp, "\t\"export-size\": %" PRIi64 ",\n", size);
fprintf (fp, "\t\"export-size-str\": \"%s\"\n", size_str);
if (last)
--
2.41.0
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On 21.09.23 19:43, Richard W.M. Jones wrote: On Thu, Sep 21, 2023 at 06:24:40PM +0200, Lee Garrett wrote: On 21.09.23 18:08, Richard W.M. Jones wrote: On Thu, Sep 21, 2023 at 04:50:26PM +0200, Lee Garrett wrote: After testing it again today, the guest agent is still not installed, however this time the C:\Program Files\Guestfs\Firstboot\log.txt exists: starting firstboot service running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0001-wait-pnp.bat" 1 file(s) moved. Wait for PnP to complete exit code 0 running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0002-install-qemu-ga-x86_64-msi-ps1.bat" 1 file(s) moved. Removing any previously scheduled qemu-ga installation ERROR: The system cannot find the file specified. Scheduling delayed installation of qemu-ga from qemu-ga-x86_64.msi SUCCESS: The scheduled task "Firstboot-qemu-ga" has successfully been created. exit code 0 running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0003-uninstall-VMware-Tools.bat" 1 file(s) moved. uninstalling VMware Tools exit code 0 uninstalling firstboot service Service uninstalled successfully The folder scripts is empty, all scripts are in scripts-done. The contents of the folder are: 5000-0001-wait-pnp.bat 123 5000-0001-wait-pnp.log 5000-0002-install-qemu-ga-x86_64-msi-ps1.bat 5000-0003-uninstall-VMware-Tools.bat 5000-0003-uninstall-VMware-Tools.log Note that the .log is missing for qemu-ga. This is what we'd expect, except maybe the missing log for qemu-ga. It's a little odd that it is missing, but it's also possible that the log file doesn't get written if there's no output. At least today I could twice provision the windows 11 VM, and in both cases the main log.txt was generated by the time I checked. How are the services scheduled? Could it be that they were delayed by > 1h? Yes, we do actually try to delay qemu-ga installation. It should only be delayed for about 120 seconds (not 1 hour). https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L580 However there have been timezone bugs with this in the past (and maybe there still are), see the sordid history in these two links: https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L558 I see. That's not reassuring. :( Which TZ are you in? The host machine is UTC+2 (Berlin summer time), however the Windows 11 trial vmware image itself defaults to UTC-8 (Pacific Time US & Canada). After a little bit of digging this indeed seems to be the issue. `schtasks.exe /Query` gives: PS C:\Users\User> schtasks.exe /Query Folder: \ TaskName Next Run Time Status == === Firstboot-qemu-ga9/21/2023 4:04:00 PM Ready [...] which is ~7 hours in the future. Btw, running `.\5000-0002-install-qemu-ga-x86_64-msi-ps1.bat` by hand works just fine. So it definitely is just the scheduling issue that's preventing it from running. So this is probably another instance or variation of the timezone formatting problem (of schtasks). Which version of virt-v2v is this? I want to check that you have a version with all the latest patches in this area. It's 2.2.0-1 from Debian (12) bookworm. I've verified that it doesn't have any distro-specific patches. (https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian would have a patches/series file in this case) Rich. Rich. On 11.09.23 21:29, Richard W.M. Jones wrote: If we suspect that the whole firstboot mechanism might not be working with the new version of Windows, one way to test it (on this one, or a freshly installed Windows VM) would be: $ virt-customize -a windows.img --firstboot-command 'echo hello' and see if "hello" is written in some form to the log.txt file inside the guest after it boots. If that doesn't work then it's likely some change in Windows which is breaking firstboot support. Rich. Regards, Lee Regards, Lee ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On Thu, Sep 21, 2023 at 06:24:40PM +0200, Lee Garrett wrote: > On 21.09.23 18:08, Richard W.M. Jones wrote: > >On Thu, Sep 21, 2023 at 04:50:26PM +0200, Lee Garrett wrote: > >>After testing it again today, the guest agent is still not > >>installed, however this time the C:\Program > >>Files\Guestfs\Firstboot\log.txt exists: > >> > >>starting firstboot service > >>running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0001-wait-pnp.bat" > >> 1 file(s) moved. > >>Wait for PnP to complete > >> exit code 0 > >>running "C:\Program > >>Files\Guestfs\Firstboot\scripts\5000-0002-install-qemu-ga-x86_64-msi-ps1.bat" > >> 1 file(s) moved. > >>Removing any previously scheduled qemu-ga installation > >>ERROR: The system cannot find the file specified. > >>Scheduling delayed installation of qemu-ga from qemu-ga-x86_64.msi > >>SUCCESS: The scheduled task "Firstboot-qemu-ga" has successfully been > >>created. > >> exit code 0 > >>running "C:\Program > >>Files\Guestfs\Firstboot\scripts\5000-0003-uninstall-VMware-Tools.bat" > >> 1 file(s) moved. > >>uninstalling VMware Tools > >> exit code 0 > >>uninstalling firstboot service > >>Service uninstalled successfully > >> > >> > >>The folder scripts is empty, all scripts are in scripts-done. The > >>contents of the folder are: > >>5000-0001-wait-pnp.bat > >>123 5000-0001-wait-pnp.log > >>5000-0002-install-qemu-ga-x86_64-msi-ps1.bat > >>5000-0003-uninstall-VMware-Tools.bat > >>5000-0003-uninstall-VMware-Tools.log > >> > >>Note that the .log is missing for qemu-ga. > > > >This is what we'd expect, except maybe the missing log for qemu-ga. > >It's a little odd that it is missing, but it's also possible that the > >log file doesn't get written if there's no output. > > > >>At least today I could twice provision the windows 11 VM, and in > >>both cases the main log.txt was generated by the time I checked. How > >>are the services scheduled? Could it be that they were delayed by > > >>1h? > > > >Yes, we do actually try to delay qemu-ga installation. It should only > >be delayed for about 120 seconds (not 1 hour). > > > >https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L580 > > > >However there have been timezone bugs with this in the past (and maybe > >there still are), see the sordid history in these two links: > > > >https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L558 > > I see. That's not reassuring. :( > > > > >Which TZ are you in? > > The host machine is UTC+2 (Berlin summer time), however the Windows > 11 trial vmware image itself defaults to UTC-8 (Pacific Time US & > Canada). > > After a little bit of digging this indeed seems to be the issue. > > `schtasks.exe /Query` gives: > > PS C:\Users\User> schtasks.exe /Query > > Folder: \ > TaskName Next Run Time Status > == > === > Firstboot-qemu-ga9/21/2023 4:04:00 PM Ready > [...] > > which is ~7 hours in the future. Btw, running > `.\5000-0002-install-qemu-ga-x86_64-msi-ps1.bat` by hand works just > fine. So it definitely is just the scheduling issue that's > preventing it from running. So this is probably another instance or variation of the timezone formatting problem (of schtasks). Which version of virt-v2v is this? I want to check that you have a version with all the latest patches in this area. Rich. > > > >Rich. > > > >>On 11.09.23 21:29, Richard W.M. Jones wrote: > >>> > >>>If we suspect that the whole firstboot mechanism might not be working > >>>with the new version of Windows, one way to test it (on this one, or a > >>>freshly installed Windows VM) would be: > >>> > >>> $ virt-customize -a windows.img --firstboot-command 'echo hello' > >>> > >>>and see if "hello" is written in some form to the log.txt file inside > >>>the guest after it boots. > >>> > >>>If that doesn't work then it's likely some change in Windows which is > >>>breaking firstboot support. > >>> > >>>Rich. > >>> > >> > >>Regards, Lee > > > > Regards, Lee -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On 21.09.23 18:08, Richard W.M. Jones wrote: On Thu, Sep 21, 2023 at 04:50:26PM +0200, Lee Garrett wrote: After testing it again today, the guest agent is still not installed, however this time the C:\Program Files\Guestfs\Firstboot\log.txt exists: starting firstboot service running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0001-wait-pnp.bat" 1 file(s) moved. Wait for PnP to complete exit code 0 running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0002-install-qemu-ga-x86_64-msi-ps1.bat" 1 file(s) moved. Removing any previously scheduled qemu-ga installation ERROR: The system cannot find the file specified. Scheduling delayed installation of qemu-ga from qemu-ga-x86_64.msi SUCCESS: The scheduled task "Firstboot-qemu-ga" has successfully been created. exit code 0 running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0003-uninstall-VMware-Tools.bat" 1 file(s) moved. uninstalling VMware Tools exit code 0 uninstalling firstboot service Service uninstalled successfully The folder scripts is empty, all scripts are in scripts-done. The contents of the folder are: 5000-0001-wait-pnp.bat 123 5000-0001-wait-pnp.log 5000-0002-install-qemu-ga-x86_64-msi-ps1.bat 5000-0003-uninstall-VMware-Tools.bat 5000-0003-uninstall-VMware-Tools.log Note that the .log is missing for qemu-ga. This is what we'd expect, except maybe the missing log for qemu-ga. It's a little odd that it is missing, but it's also possible that the log file doesn't get written if there's no output. At least today I could twice provision the windows 11 VM, and in both cases the main log.txt was generated by the time I checked. How are the services scheduled? Could it be that they were delayed by > 1h? Yes, we do actually try to delay qemu-ga installation. It should only be delayed for about 120 seconds (not 1 hour). https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L580 However there have been timezone bugs with this in the past (and maybe there still are), see the sordid history in these two links: https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L558 I see. That's not reassuring. :( Which TZ are you in? The host machine is UTC+2 (Berlin summer time), however the Windows 11 trial vmware image itself defaults to UTC-8 (Pacific Time US & Canada). After a little bit of digging this indeed seems to be the issue. `schtasks.exe /Query` gives: PS C:\Users\User> schtasks.exe /Query Folder: \ TaskName Next Run Time Status == === Firstboot-qemu-ga9/21/2023 4:04:00 PM Ready [...] which is ~7 hours in the future. Btw, running `.\5000-0002-install-qemu-ga-x86_64-msi-ps1.bat` by hand works just fine. So it definitely is just the scheduling issue that's preventing it from running. Rich. On 11.09.23 21:29, Richard W.M. Jones wrote: If we suspect that the whole firstboot mechanism might not be working with the new version of Windows, one way to test it (on this one, or a freshly installed Windows VM) would be: $ virt-customize -a windows.img --firstboot-command 'echo hello' and see if "hello" is written in some form to the log.txt file inside the guest after it boots. If that doesn't work then it's likely some change in Windows which is breaking firstboot support. Rich. Regards, Lee Regards, Lee ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
On Thu, Sep 21, 2023 at 04:50:26PM +0200, Lee Garrett wrote: > After testing it again today, the guest agent is still not > installed, however this time the C:\Program > Files\Guestfs\Firstboot\log.txt exists: > > starting firstboot service > running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0001-wait-pnp.bat" > 1 file(s) moved. > Wait for PnP to complete > exit code 0 > running "C:\Program > Files\Guestfs\Firstboot\scripts\5000-0002-install-qemu-ga-x86_64-msi-ps1.bat" > 1 file(s) moved. > Removing any previously scheduled qemu-ga installation > ERROR: The system cannot find the file specified. > Scheduling delayed installation of qemu-ga from qemu-ga-x86_64.msi > SUCCESS: The scheduled task "Firstboot-qemu-ga" has successfully been created. > exit code 0 > running "C:\Program > Files\Guestfs\Firstboot\scripts\5000-0003-uninstall-VMware-Tools.bat" > 1 file(s) moved. > uninstalling VMware Tools > exit code 0 > uninstalling firstboot service > Service uninstalled successfully > > > The folder scripts is empty, all scripts are in scripts-done. The > contents of the folder are: > 5000-0001-wait-pnp.bat > 123 5000-0001-wait-pnp.log > 5000-0002-install-qemu-ga-x86_64-msi-ps1.bat > 5000-0003-uninstall-VMware-Tools.bat > 5000-0003-uninstall-VMware-Tools.log > > Note that the .log is missing for qemu-ga. This is what we'd expect, except maybe the missing log for qemu-ga. It's a little odd that it is missing, but it's also possible that the log file doesn't get written if there's no output. > At least today I could twice provision the windows 11 VM, and in > both cases the main log.txt was generated by the time I checked. How > are the services scheduled? Could it be that they were delayed by > > 1h? Yes, we do actually try to delay qemu-ga installation. It should only be delayed for about 120 seconds (not 1 hour). https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L580 However there have been timezone bugs with this in the past (and maybe there still are), see the sordid history in these two links: https://github.com/libguestfs/libguestfs-common/blob/e70d89a58dae068be2e19c7c21558707261af96a/mlcustomize/inject_virtio_win.ml#L558 Which TZ are you in? Rich. > On 11.09.23 21:29, Richard W.M. Jones wrote: > > > >If we suspect that the whole firstboot mechanism might not be working > >with the new version of Windows, one way to test it (on this one, or a > >freshly installed Windows VM) would be: > > > > $ virt-customize -a windows.img --firstboot-command 'echo hello' > > > >and see if "hello" is written in some form to the log.txt file inside > >the guest after it boots. > > > >If that doesn't work then it's likely some change in Windows which is > >breaking firstboot support. > > > >Rich. > > > > Regards, Lee -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [External] Re: [PATCH] VxFS Filesystem support to libguestfs
Hi Richard,
Thank you for making these changes. We appreciate your insights. We will
promptly address the issues you have mentioned, thoroughly test them.
We will keep you updated and send you the patch once we have completed the
testing.
Thanks,
Ravi/Gaurang
From: Richard W.M. Jones
Sent: Tuesday, September 19, 2023 4:30 PM
To: Ravi Singh
Cc: libguestfs@redhat.com ; Aswad Kulkarni
; Shailesh Marathe ;
Sumit Dighe ; Mitul Kothari
; Brad Boyer ; Saket
Pusalkar ; Gaurang Agnihotri
; Satyajit Gorhe parlikar
; John Cronin
Subject: Re: [External] Re: [Libguestfs] [PATCH] VxFS Filesystem support to
libguestfs
CAUTION: This email originated from outside the organization. Do not click
links or open attachments unless you recognize the sender and know the content
is safe. If you believe this is a phishing email, use the Report to
Cybersecurity icon in Outlook.
A new patch is attached. This is only build tested so far.
To build with this patch I had to do:
./configure --enable-werror \
--with-supermin-packager-config=$PWD/localyum.conf
where localyum.conf contains:
--
[local]
name=local
baseurl=file:///root/dvd1-redhatlinux/rhel9_x86_64/rpms/
enabled=1
gpgcheck=0
--
(adjust the path to point to the Veritas DVD mount point)
I made various changes in this patch:
- Add copyright and license notice to list_vxvm.ml & vxfs.c. Please
ensure all files have copyright and license notices, and if necessary
get them checked by your legal.
- Change vm.ml{,i} -> list_vxvm.ml{,i}
- In packagelist.in, move package names to their own section.
- Remove perl/python packages as perl/python is not present in
the appliance.
- Fix commit message.
- Line length under 80 columns in a few places.
- Function names at start of line.
- List SOURCES_MLI alphabetically.
- daemon/listfs.ml: Remove comment about has_vxvm.
- do_vxfs_start -> vxfs_start (since do_* is reserved prefix in daemon).
- Skip vxfs_start if !vxdisk available, to save start up time.
- Add a prototype for vxfs_start.
- Don't use reply_with_error in vxfs_start, as we're not called
during protocol context.
- Use only one *err.
- Add daemon/list_vxvm.mli & daemon/vxvm_type.mli to .gitignore
vxfs_start has several issues which need to be addressed:
- Why does it return if a service (eg. veki) is not available? That
would prevent all further initialization.
- I think this would be better done as part of appliance/init, and
delete this function completely.
Rich.
--
Richard Jones, Virtualization Group, Red Hat
https://nam12.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpeople.redhat.com%2F~rjones=05%7C01%7Cravi.singh%40veritas.com%7C6df598eb4c58442388ef08dbb8ffa410%7Cfc8e13c0422c4c55b3eaca318e6cac32%7C0%7C0%7C638307180357901926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=oNbYsYG%2B0kJX%2FSUyTgba10nH8762rEvcEWqN1n4tlHs%3D=0
Read my programming and virtualization blog:
https://nam12.safelinks.protection.outlook.com/?url=http%3A%2F%2Frwmj.wordpress.com%2F=05%7C01%7Cravi.singh%40veritas.com%7C6df598eb4c58442388ef08dbb8ffa410%7Cfc8e13c0422c4c55b3eaca318e6cac32%7C0%7C0%7C638307180357901926%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=LP%2BLKvr3i%2Bc%2BTbIln8uIdakIrWhbYyOW8reELLnIvqE%3D=0
[Libguestfs] Some comments on the libnbd apk in Alpine
Hi Sean, the maintainers of libnbd here. Thanks for packaging libnbd in Alpine: https://git.alpinelinux.org/aports/tree/testing/libnbd/APKBUILD I have a few comments: (1) The url field should be "https://gitlab.com/nbdkit/libnbd; (The one you are using is an old, stale mirror) (2) libxml2-dev is not listed as a dependency. We had a request from an Alpine user to add this dependency because it enables several important features in libnbd, namely NBD URI support and some utilities which require URI support. It shouldn't be too much trouble to add since it appears that libxml2 is already built in Alpine. (3) libnbd 1.18 (next stable) will be released in a week or two: https://listman.redhat.com/archives/libguestfs/2023-September/032590.html If you have any questions or comments about the package you can send them to me and/or the mailing list (without subscribing if you don't want to). 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 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot
After testing it again today, the guest agent is still not installed, however this time the C:\Program Files\Guestfs\Firstboot\log.txt exists: starting firstboot service running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0001-wait-pnp.bat" 1 file(s) moved. Wait for PnP to complete exit code 0 running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0002-install-qemu-ga-x86_64-msi-ps1.bat" 1 file(s) moved. Removing any previously scheduled qemu-ga installation ERROR: The system cannot find the file specified. Scheduling delayed installation of qemu-ga from qemu-ga-x86_64.msi SUCCESS: The scheduled task "Firstboot-qemu-ga" has successfully been created. exit code 0 running "C:\Program Files\Guestfs\Firstboot\scripts\5000-0003-uninstall-VMware-Tools.bat" 1 file(s) moved. uninstalling VMware Tools exit code 0 uninstalling firstboot service Service uninstalled successfully The folder scripts is empty, all scripts are in scripts-done. The contents of the folder are: 5000-0001-wait-pnp.bat 123 5000-0001-wait-pnp.log 5000-0002-install-qemu-ga-x86_64-msi-ps1.bat 5000-0003-uninstall-VMware-Tools.bat 5000-0003-uninstall-VMware-Tools.log Note that the .log is missing for qemu-ga. At least today I could twice provision the windows 11 VM, and in both cases the main log.txt was generated by the time I checked. How are the services scheduled? Could it be that they were delayed by > 1h? On 11.09.23 21:29, Richard W.M. Jones wrote: If we suspect that the whole firstboot mechanism might not be working with the new version of Windows, one way to test it (on this one, or a freshly installed Windows VM) would be: $ virt-customize -a windows.img --firstboot-command 'echo hello' and see if "hello" is written in some form to the log.txt file inside the guest after it boots. If that doesn't work then it's likely some change in Windows which is breaking firstboot support. Rich. Regards, Lee ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] regression: file does not understand the -S option
Thu, 21 Sep 2023 10:21:41 +0200 Laszlo Ersek : > Does your error output contain > file: invalid option -- 'S' > ? Yes, it looks like this: # /usr/bin/env -i /usr/bin/file -S /usr/bin/file: invalid option -- 'S' Usage: file [-bcEhikLlNnprsvzZ0] [--apple] [--extension] [--mime-encoding] [--mime-type] [-e testname] [-F separator] [-f namefile] [-m magicfiles] file ... file -C [-m magicfiles] file [--help] Olaf pgpKM1Gr88hFg.pgp Description: Digitale Signatur von OpenPGP ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libguestfs] daemon: Omit 'file -S' option on older distros that lack support
This is upstream in c95d8c4cf64142. Also added a few commits to fix the Ruby tests which had broken because of a recent change to Ruby. I'll cut a development release soon. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libguestfs] daemon: Omit 'file -S' option on older distros that lack support
On Thu, Sep 21, 2023 at 03:56:05PM +0200, Laszlo Ersek wrote: ... > > +open Std_utils > > + > > +(* Does [file] support the [-S] / [--no-sandbox] option > > + * (not on OpenSUSE LEAP 15). > > + *) > > +let file_has_S_option = lazy ( > > + let out = Utils.command "file" ["file"; "--help"] in > > I think this executes "file file --help", which happens to work, but the > intent is probably just "file --help". (IOW, IMO this should be > > Utils.command "file" ["--help"] > > .) Indeed it does. BTW I did check this patch by running: $ make && LIBGUESTFS_DEBUG=1 LIBGUESTFS_TRACE=1 make -C tests/ check TESTS=c-api/tests and examining tests/c-api/tests.log, and that shows that file --help (or rather, "file file --help") is being run exactly once, and the -S option was still being included in the parameters. > Reviewed-by: Laszlo Ersek Thanks, I'll tidy up and fix the things you mentioned. 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 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libguestfs] daemon: Omit 'file -S' option on older distros that lack support
On 9/21/23 13:42, Richard W.M. Jones wrote: > OpenSUSE LEAP 15 lacks support for this option, so test for it before > using it. > > See-also: > https://listman.redhat.com/archives/libguestfs/2023-September/032613.html > Report-by: Olaf Hering > Fixes: commit 23986d3c4f4d1f9cbac44cc743d3e6af721e4237 > --- > daemon/Makefile.am | 2 ++ > daemon/file.ml | 10 -- > daemon/file_helper.ml | 29 + > daemon/file_helper.mli | 19 +++ > daemon/filearch.ml | 5 - > 5 files changed, 62 insertions(+), 3 deletions(-) > > diff --git a/daemon/Makefile.am b/daemon/Makefile.am > index bb2e58d014..01c0f6416c 100644 > --- a/daemon/Makefile.am > +++ b/daemon/Makefile.am > @@ -280,6 +280,7 @@ SOURCES_MLI = \ > devsparts.mli \ > file.mli \ > filearch.mli \ > + file_helper.mli \ > findfs.mli \ > inspect.mli \ > inspect_fs.mli \ > @@ -321,6 +322,7 @@ SOURCES_ML = \ > btrfs.ml \ > cryptsetup.ml \ > devsparts.ml \ > + file_helper.ml \ > file.ml \ > filearch.ml \ > isoinfo.ml \ > diff --git a/daemon/file.ml b/daemon/file.ml > index 1f87b190c1..f0ef181938 100644 > --- a/daemon/file.ml > +++ b/daemon/file.ml > @@ -43,7 +43,10 @@ let file path = > | S_SOCK -> "socket" > | S_REG -> > (* Regular file, so now run [file] on it. *) > - let out = command "file" ["-zSb"; Sysroot.sysroot_path path] in > + let file_options = > + sprintf "-z%sb" > + (if File_helper.file_has_S_option () then "S" else "") in > + let out = command "file" [file_options; Sysroot.sysroot_path path] in > > (* We need to remove the trailing \n from output of file(1). > * This still open-codes "-z" and "-b" in multiple places, plus the file_has_S_option() call; I thought of a somewhat thicker wrapper (hiding all the details / options behind the "file" and "file-architecture" APIs). But, this works as well of course. > @@ -54,6 +57,9 @@ let file path = > String.trimr out >) >else (* it's a device *) ( > -let out = command "file" ["-zSbsL"; path] in > +let file_options = > + sprintf "-z%sbsL" > +(if File_helper.file_has_S_option () then "S" else "") in > +let out = command "file" [file_options; path] in > String.trimr out >) > diff --git a/daemon/file_helper.ml b/daemon/file_helper.ml > new file mode 100644 > index 00..f8c4bcbe56 > --- /dev/null > +++ b/daemon/file_helper.ml > @@ -0,0 +1,29 @@ > +(* guestfs-inspection "guestfs-inspection" -- strange, but I see it in a bunch of other places. > + * Copyright (C) 2009-2023 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 > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + *) > + > +open Std_utils > + > +(* Does [file] support the [-S] / [--no-sandbox] option > + * (not on OpenSUSE LEAP 15). > + *) > +let file_has_S_option = lazy ( > + let out = Utils.command "file" ["file"; "--help"] in I think this executes "file file --help", which happens to work, but the intent is probably just "file --help". (IOW, IMO this should be Utils.command "file" ["--help"] .) > + String.find out "--no-sandbox" >= 0 > + > +) > +let file_has_S_option () = Lazy.force file_has_S_option > diff --git a/daemon/file_helper.mli b/daemon/file_helper.mli > new file mode 100644 > index 00..a644cf6de2 > --- /dev/null > +++ b/daemon/file_helper.mli > @@ -0,0 +1,19 @@ > +(* guestfs-inspection > + * Copyright (C) 2009-2023 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 > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + *) > + >
[Libguestfs] [PATCH libguestfs] daemon: Omit 'file -S' option on older distros that lack support
OpenSUSE LEAP 15 lacks support for this option, so test for it before using it. See-also: https://listman.redhat.com/archives/libguestfs/2023-September/032613.html Report-by: Olaf Hering Fixes: commit 23986d3c4f4d1f9cbac44cc743d3e6af721e4237 --- daemon/Makefile.am | 2 ++ daemon/file.ml | 10 -- daemon/file_helper.ml | 29 + daemon/file_helper.mli | 19 +++ daemon/filearch.ml | 5 - 5 files changed, 62 insertions(+), 3 deletions(-) diff --git a/daemon/Makefile.am b/daemon/Makefile.am index bb2e58d014..01c0f6416c 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -280,6 +280,7 @@ SOURCES_MLI = \ devsparts.mli \ file.mli \ filearch.mli \ + file_helper.mli \ findfs.mli \ inspect.mli \ inspect_fs.mli \ @@ -321,6 +322,7 @@ SOURCES_ML = \ btrfs.ml \ cryptsetup.ml \ devsparts.ml \ + file_helper.ml \ file.ml \ filearch.ml \ isoinfo.ml \ diff --git a/daemon/file.ml b/daemon/file.ml index 1f87b190c1..f0ef181938 100644 --- a/daemon/file.ml +++ b/daemon/file.ml @@ -43,7 +43,10 @@ let file path = | S_SOCK -> "socket" | S_REG -> (* Regular file, so now run [file] on it. *) - let out = command "file" ["-zSb"; Sysroot.sysroot_path path] in + let file_options = + sprintf "-z%sb" + (if File_helper.file_has_S_option () then "S" else "") in + let out = command "file" [file_options; Sysroot.sysroot_path path] in (* We need to remove the trailing \n from output of file(1). * @@ -54,6 +57,9 @@ let file path = String.trimr out ) else (* it's a device *) ( -let out = command "file" ["-zSbsL"; path] in +let file_options = + sprintf "-z%sbsL" +(if File_helper.file_has_S_option () then "S" else "") in +let out = command "file" [file_options; path] in String.trimr out ) diff --git a/daemon/file_helper.ml b/daemon/file_helper.ml new file mode 100644 index 00..f8c4bcbe56 --- /dev/null +++ b/daemon/file_helper.ml @@ -0,0 +1,29 @@ +(* guestfs-inspection + * Copyright (C) 2009-2023 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 + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + *) + +open Std_utils + +(* Does [file] support the [-S] / [--no-sandbox] option + * (not on OpenSUSE LEAP 15). + *) +let file_has_S_option = lazy ( + let out = Utils.command "file" ["file"; "--help"] in + String.find out "--no-sandbox" >= 0 + +) +let file_has_S_option () = Lazy.force file_has_S_option diff --git a/daemon/file_helper.mli b/daemon/file_helper.mli new file mode 100644 index 00..a644cf6de2 --- /dev/null +++ b/daemon/file_helper.mli @@ -0,0 +1,19 @@ +(* guestfs-inspection + * Copyright (C) 2009-2023 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 + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + *) + +val file_has_S_option : unit -> bool diff --git a/daemon/filearch.ml b/daemon/filearch.ml index 7c858129db..cf784f18a2 100644 --- a/daemon/filearch.ml +++ b/daemon/filearch.ml @@ -128,7 +128,10 @@ and cpio_arch magic orig_path path = | bin :: bins -> let bin_path = tmpdir // bin in if is_regular_file bin_path then ( - let out = command "file" ["-zSb"; bin_path] in + let file_options = + sprintf "-z%sb" + (if File_helper.file_has_S_option () then "S" else "") in + let out = command "file" [file_options; bin_path] in file_architecture_of_magic out orig_path bin_path ) else -- 2.41.0
Re: [Libguestfs] regression: file does not understand the -S option
On Thu, Sep 21, 2023 at 12:25:21PM +0100, Richard W.M. Jones wrote: > On Wed, Sep 20, 2023 at 11:42:55PM +0200, Olaf Hering wrote: > > Recently a commit was added to call 'file -zSb' instead of 'file -zb'. > > > > This causes a regression on Leap 15 (but not on Tumbleweed), because > > file 5.32 does not understand the -S option. > > > > How can this be fixed properly, to handle both cases either at runtime > > or at buildtime? > > The background to this was: > > https://github.com/libguestfs/libguestfs/issues/100 > > It took a while to work out what was going on in the original bug > report, but it turned out that Arch (IIRC) enabled the seccomp feature > in the 'file' command. This filters what system calls 'file' is > allowed to make, which strengthens security as 'file' is often run on > untrusted inputs. > > Unfortunately the seccomp rules for 'file' don't cope with running > external programs (ie. 'file -z' which runs zcat). We filed a bug to > try to get that fixed: > > https://bugzilla.redhat.com/show_bug.cgi?id=2148753 > https://bugs.astron.com/view.php?id=406 > > but the fix to seccomp policy was rejected recently in both Fedora & > upstream. Their rationale in that bug makes no sense. Not allowing 'clone+execve' etc is correct when '-z' is NOT specified by the user. No argument there. If '-z' is specified then adding clone+execve etc is the only way it can work. They should apply a different seccomp filter for '-z' only which includes clone+execve, etc. Telling people to turn off seccomp entirely in order to use '-z' is even worse for security than just allowing clone+execve. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] regression: file does not understand the -S option
On Wed, Sep 20, 2023 at 11:42:55PM +0200, Olaf Hering wrote: > Recently a commit was added to call 'file -zSb' instead of 'file -zb'. > > This causes a regression on Leap 15 (but not on Tumbleweed), because > file 5.32 does not understand the -S option. > > How can this be fixed properly, to handle both cases either at runtime > or at buildtime? The background to this was: https://github.com/libguestfs/libguestfs/issues/100 It took a while to work out what was going on in the original bug report, but it turned out that Arch (IIRC) enabled the seccomp feature in the 'file' command. This filters what system calls 'file' is allowed to make, which strengthens security as 'file' is often run on untrusted inputs. Unfortunately the seccomp rules for 'file' don't cope with running external programs (ie. 'file -z' which runs zcat). We filed a bug to try to get that fixed: https://bugzilla.redhat.com/show_bug.cgi?id=2148753 https://bugs.astron.com/view.php?id=406 but the fix to seccomp policy was rejected recently in both Fedora & upstream. The patch we added to libguestfs turns off seccomp sandboxing, both because it's broken (see above) and because we don't really need it as we run stuff in a virtual machine already: https://github.com/libguestfs/libguestfs/commit/23986d3c4f4d1f9cbac44cc743d3e6af721e4237 I didn't realise there were distros that lack support for the 'file -S' option. So I guess the fix is to detect if 'file' has the -S option ... I think we can just grep 'file --help' for the -S / --no-sandbox option. Let me try for a patch now. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [EXTERNAL] - Re: LIBGUESTFS mount disk failure
Hi @Richard W.M. Jones, I am attaching a sample program of how we are using guestfs apis where we are getting this error guestfs_mount_local fail. Please take a look into this. Thanks, Divyanshu -Original Message- From: Richard W.M. Jones Sent: Tuesday, September 19, 2023 5:36 PM To: Raja Ram Sharma Cc: Teja Konapalli ; libguestfs@redhat.com; Divyanshu Kumar Subject: Re: [EXTERNAL] - Re: LIBGUESTFS mount disk failure CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. If you feel that the email is suspicious, please report it using PhishAlarm. On Tue, Sep 19, 2023 at 11:59:43AM +, Raja Ram Sharma wrote: > Hi Richard, > > Thank you for the response, will set both handler and let you know. > > when we try to execute guestfs_mount_local. > Getting below error : > > [ 2023-09-19 13:34:05 ] errno: ( 0 ) : mount_local: unknown option > 554101136 (this can happen if a program is compiled against a newer > version of libguestfs, then dynamically linked to an older version) I attach what I said previously. Rich. > Setup details : > OS : (PRETTY_NAME="Red Hat Enterprise Linux 8.2 (Ootpa)") Guesfs.h > version : 1.50 Guestfish version : guestfish > 1.38.4rhel=8,release=15.module+el8.2.0+5297+222a20af,libvirt > > In Below scenario: > 1. guestfs_mount_local_run > 2. umount /mountpoint > 3. Again, while trying to mount, after sleep of 20sec. > > > Thanks > RR > > > > > > -Original Message- > From: Richard W.M. Jones > Sent: Tuesday, September 19, 2023 1:15 PM > To: Raja Ram Sharma > Cc: Teja Konapalli ; libguestfs@redhat.com; > Divyanshu Kumar > Subject: Re: [EXTERNAL] - Re: LIBGUESTFS mount disk failure > > CAUTION: This email originated from outside of the organization. Do not click > links or open attachments unless you recognize the sender and know the > content is safe. If you feel that the email is suspicious, please report it > using PhishAlarm. > > > On Sun, Sep 17, 2023 at 04:11:25PM +, Raja Ram Sharma wrote: > > One more help is needed : > > https://urldefense.com/v3/__https://github.com/libguestfs/libguestfs > > /i > > ssues/124__;!!Obbck6kTJA!blek-uFsj3xPSaSiNQrS-1lk59I6-udl4zqOJCxZ6LL > > eu pvAFl6tPfod_GU2BLm_19gJUs3OXp6_IUEM$ > > /***/ > > I can get guestfs_last_error using function_callback and write to log file. > > But "event Handling" is not working Also, std::cout and std::cerr still > > printing on terminal. > > Is there any workaround for Event Handling? > > How to completely disable/enable standard output? > > /***/ > > > > In short, Any Debugging sample program, > > https://urldefense.com/v3/__https://github.com/libguestfs/libguestfs > > /i > > ssues/124__;!!Obbck6kTJA!blek-uFsj3xPSaSiNQrS-1lk59I6-udl4zqOJCxZ6LL > > eu pvAFl6tPfod_GU2BLm_19gJUs3OXp6_IUEM$ > > In the two programs in that issue, the first program sets an event callback > and the second program sets an error handler. To capture all messages you > must set both together. > > If you can produce a single, short reproducer which uses both types of > handlers and still prints to stderr, then I can look at that. > > Rich. > > -- > Richard Jones, Virtualization Group, Red Hat > https://urldefense.com/v3/__http://people.redhat.com/*rjones__;fg!!Obb > ck6kTJA!blek-uFsj3xPSaSiNQrS-1lk59I6-udl4zqOJCxZ6LLeupvAFl6tPfod_GU2BL > m_19gJUs3OXuHRnZTv$ Read my programming and virtualization blog: > https://urldefense.com/v3/__http://rwmj.wordpress.com__;!!Obbck6kTJA!b > lek-uFsj3xPSaSiNQrS-1lk59I6-udl4zqOJCxZ6LLeupvAFl6tPfod_GU2BLm_19gJUs3 > OXrVS1VaN$ Fedora Windows cross-compiler. Compile Windows programs, > test, and build Windows installers. Over 100 libraries supported. > https://urldefense.com/v3/__http://fedoraproject.org/wiki/MinGW__;!!Ob > bck6kTJA!blek-uFsj3xPSaSiNQrS-1lk59I6-udl4zqOJCxZ6LLeupvAFl6tPfod_GU2B > Lm_19gJUs3OXjCvHvVp$ -- Richard Jones, Virtualization Group, Red Hat https://urldefense.com/v3/__http://people.redhat.com/*rjones__;fg!!Obbck6kTJA!ZE9Jts1w7JGiKHKAcLv1I72RncLG2NUVxKSn4yXEUiITBywqaqIgR4po1KK5rdHojo9l36xX1V-CKYfN$ Read my programming and virtualization blog: https://urldefense.com/v3/__http://rwmj.wordpress.com__;!!Obbck6kTJA!ZE9Jts1w7JGiKHKAcLv1I72RncLG2NUVxKSn4yXEUiITBywqaqIgR4po1KK5rdHojo9l36xX1WGJ1lVR$ Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. https://urldefense.com/v3/__http://fedoraproject.org/wiki/MinGW__;!!Obbck6kTJA!ZE9Jts1w7JGiKHKAcLv1I72RncLG2NUVxKSn4yXEUiITBywqaqIgR4po1KK5rdHojo9l36xX1XNZvfww$ #include #include #include #include #include #include #include #include #include #include #include #include #include #include #include #include #include #define FILESYSTEM "filesystem" using namespace std; std::fstream log_file; void message_callback (guestfs_h *g, void
Re: [Libguestfs] regression: file does not understand the -S option
On 9/20/23 23:42, Olaf Hering wrote:
> Recently a commit was added to call 'file -zSb' instead of 'file -zb'.
>
> This causes a regression on Leap 15 (but not on Tumbleweed), because
> file 5.32 does not understand the -S option.
>
> How can this be fixed properly, to handle both cases either at runtime
> or at buildtime?
This is likely from commit 23986d3c4f4d ("file: Use -S option with -z",
2022-11-28).
Does your error output contain
file: invalid option -- 'S'
?
If it does, then I think we could modify "daemon/file.ml" and
"daemon/filearch.ml". Try "file" with the current options, and if
there's a failure, and stderr contains the above string, retry without -S.
Unfortunately, this is a bit messy. We'd probably want to cache the
availability of -S. Also, because this logic is used from multiple
places, we'd first have to factor out the current "file" invocation --
minimally, try to rebase the "file" invocation in "daemon/filearch.ml"
to the interface exposed by "daemon/file.mli".
Laszlo
>
>
> Thanks,
> Olaf
>
>
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
