Re: [PATCH 33/38] trace-cmd record: prevent memory corruption in parse_record_options()

[Steven Rostedt <rostedt@goodmis.org>]

On Wed,  5 Jun 2024 15:40:48 +0200
"Jerome Marchand" <jmarchan@redhat.com> wrote:

> In parse_record_options() we can end up using a deleted instance after
> options have been parsed. This can be triggered by the following
> command:
> $ trace-cmd record -v -e block -B foo  ls
> 
> We probably need a proper to avoid to end up in this situation, but in
> the mean time, check that the current instance isn't marked for
> deletion before calling remove_instances(). That at least prevent an
> hard to debug memory corruption bug.
> 
> Fixes a USE_AFTER_FREE error (CWE-416)
> 
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>  tracecmd/trace-record.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 770e775b..dc3e5285 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -6909,6 +6909,8 @@ static void parse_record_options(int argc,
>  		}
>  	}
>  
> +	if (ctx->instance->delete)
> +		die("Instance to be deleted is still used");

This looks to only be an issue for record. Deletion of instances should
only be for the trace-cmd set command.

>  	remove_instances(del_list);
>  
>  	/* If --date is specified, prepend it to all guest VM flags */

I'll add this patch:

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 4e9ac598..1527be11 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -6748,7 +6748,8 @@ static void parse_record_options(int argc,
 			ctx->instance = allocate_instance(optarg);
 			if (!ctx->instance)
 				die("Failed to create instance");
-			ctx->instance->delete = negative;
+			if (IS_CMDSET(ctx))
+				ctx->instance->delete = negative;
 			negative = 0;
 			if (ctx->instance->delete) {
 				ctx->instance->next = del_list;

Which should fix the issue as well.

Thanks,

-- Steve