Re: [PULL 19/31] migration: Normalize tls arguments

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Apr 14, 2026 at 06:56:27PM +0200, Maciej S. Szmigiero wrote:
>> On 23.12.2025 15:29, Peter Xu wrote:
>> > From: Fabiano Rosas <farosas@suse.de>
>> > 
>> > The migration parameters tls_creds, tls_authz and tls_hostname
>> > currently have a non-uniform handling. When used as arguments to
>> > migrate-set-parameters, their type is StrOrNull and when used as
>> > return value from query-migrate-parameters their type is a plain
>> > string.
>> > 
>> > Not only having to convert between the types is cumbersome, but it
>> > also creates the issue of requiring two different QAPI types to be
>> > used, one for each command. MigrateSetParameters is used for
>> > migrate-set-parameters with the TLS arguments as StrOrNull while
>> > MigrationParameters is used for query-migrate-parameters with the TLS
>> > arguments as str.
>> > 
>> > Since StrOrNull could be considered a superset of str, change the type
>> > of the TLS arguments in MigrationParameters to StrOrNull. Also ensure
>> > that QTYPE_QNULL is never used.
>> > 
>> > 1) migrate-set-parameters will always write QTYPE_QSTRING to
>> >    s->parameters, either an empty or non-empty string.
>> > 
>> > 2) query-migrate-parameters will always return a QTYPE_QSTRING, either
>> >    empty or non-empty.
>> > 
>> > 3) the migrate_tls_* helpers will always return a non-empty string or
>> >    NULL, for the internal migration code's consumption.
>> > 
>> > Points (1) and (2) above help simplify the parameters validation and
>> > the query command handling because s->parameters is already kept in
>> > the format that query-migrate-parameters (and info migrate_paramters)
>> > expect. Point (3) is so people don't need to care about StrOrNull in
>> > migration code.
>> > 
>> > This will allow the type duplication to be removed in the next
>> > patches.
>> > 
>> > Note that the type of @tls_creds, @tls-hostname, @tls-authz changes
>> > from str to StrOrNull in introspection of the query-migrate-parameters
>> > command. We accept this imprecision to enable de-duplication.
>> > 
>> > There's no need to free the TLS options in
>> > migration_instance_finalize() because they're freed by the qdev
>> > properties .release method.
>> > 
>> > Temporary in this patch:
>> > migrate_params_test_apply() copies s->parameters into a temporary
>> > structure, so it's necessary to drop the references to the TLS options
>> > if they were not set by the user to avoid double-free. This is fixed
>> > in the next patches.
>> > 
>> > Acked-by: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> > Link: https://lore.kernel.org/r/20251215220041.12657-6-farosas@suse.de
>> > [peterx: in hmp_info_migrate_parameters(), remove an extra dump of
>> >   max_postcopy_bandwidth, introduced likely by accident]
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >   qapi/migration.json            |   6 +-
>> >   migration/options.h            |   1 +
>> >   migration/migration-hmp-cmds.c |   6 +-
>> >   migration/options.c            | 144 +++++++++++++++++++--------------
>> >   migration/tls.c                |   2 +-
>> >   5 files changed, 93 insertions(+), 66 deletions(-)
>> > 
>> > diff --git a/migration/options.c b/migration/options.c
>> > index d55f3104be..6ef3c56fb6 100644
>> > --- a/migration/options.c
>> > +++ b/migration/options.c
>> (..)
>> > @@ -1243,7 +1274,7 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
>> >   #ifdef CONFIG_LINUX
>> >       if (migrate_zero_copy_send() &&
>> >           ((params->has_multifd_compression && params->multifd_compression) ||
>> > -         (params->tls_creds && *params->tls_creds))) {
>> > +         *params->tls_creds->u.s)) {
>> >           error_setg(errp,
>> >                      "Zero copy only available for non-compressed non-TLS multifd migration");
>> >           return false;
>> The above change gives me easily triggerable NULL pointer dereference:
>> > $ qemu-system-x86_64 -monitor stdio -global migration.x-multifd=true -global migration.x-zero-copy-send=true
>> > QEMU 10.2.93 monitor - type 'help' for more information
>> > VNC server running on ::1:5900
>> > (qemu) migrate_set_parameter downtime-limit 500
>> > Segmentation fault
>
> Oops..
>

Why do you guys still let me touch this codebase?

>> 
>> I guess params->tls_creds really needs that NULL check before being accessed.
>
> Yeah, my gut feeling is we got this special casing of using a temp
> parameter object..  I'll leave Fabiano to double check on that and send
> patch..
>
> Thanks for the report!

Good that I write my bugs with matching fixes to go along.

https://lore.kernel.org/r/20260202224101.20568-3-farosas@suse.de

I'll repost with a better commit message.