[DTrace-devel] [PATCH] cg, sdt: copy userspace string arguments to kernelspace

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
minor comments...

On 2/28/23 03:20, Kris Van Hees via DTrace-devel wrote:
> When an SDT probe argument is provided by a userspace string, it must be
> copied to kernelspace because SDT probe pointer arguments are expected
> to be kernel addresses.
>
> Signed-off-by: Kris Van Hees<kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_cg.c          | 31 +++++++++++++++++++++++++++++++
>   libdtrace/dt_probe.c       |  1 +
>   libdtrace/dt_prov_proc.c   | 38 +++++++++++++++++++-------------------
>   libdtrace/dt_provider.h    |  1 +
>   libdtrace/dt_provider_tp.c |  1 +
>   5 files changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 2c4648df..be0215d7 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -4132,6 +4132,7 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   	dt_ident_t	*fidp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_bvar");
>   	size_t		size;
>   	int		n;
> +	int		ustr = 0;
>   
>   	assert(dnp->dn_kind == DT_NODE_VAR);
>   	assert(!(idp->di_flags & (DT_IDFLG_TLS | DT_IDFLG_LOCAL)));
> @@ -4155,6 +4156,12 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   			dnp->dn_reg = -1;
>   			return;
>   		}
> +
> +		/*
> +		 * If the argument node is mark DT_NF_USERLAND, it is a string
> +		 * in userspace.  Set a flag to indicate we need to copy it.
> +		 */
> +		ustr = prp->xargv[saved]->dn_flags & DT_NF_USERLAND;
>   		dnp->dn_args->dn_value = prp->mapping[saved];
>   	}
>   
> @@ -4201,6 +4208,30 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   	emit(dlp, BPF_MOV_REG(dnp->dn_reg, BPF_REG_0));
>   	dt_regset_free(drp, BPF_REG_0);
>   
> +	if (dt_node_is_string(dnp) && ustr) {
> +		dtrace_hdl_t    *dtp = yypcb->pcb_hdl;
> +
> +		dt_regset_xalloc(drp, BPF_REG_0);

Why %r0?  How about xalloc_args() here and put the "src" value directly 
into %r3, where it is eventually needed?  That would save the later 
%r3=%r0 instruction.

> +
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_0, dnp->dn_reg));
> +		dt_cg_tstring_alloc(yypcb, dnp);
> +		emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_FP, DT_STK_DCTX));
> +		emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, dnp->dn_reg, DCTX_MEM));
> +		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, dnp->dn_tstring->dn_value));
> +
> +		if (dt_regset_xalloc_args(drp) == -1)
> +			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_1, dnp->dn_reg));
> +		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, dtp->dt_options[DTRACEOPT_STRSIZE]));
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_3, BPF_REG_0));
> +		emit(dlp,  BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_user_str]));
> +
> +		dt_regset_free_args(drp);
> +		dt_regset_free(drp, BPF_REG_0);
> +		return;
> +	}
> +
>   	/*
>   	 * If this is a reference to the args[] array, we need to take the
>   	 * additional step of explicitly eliminating any bits larger than the
> diff --git a/libdtrace/dt_probe.c b/libdtrace/dt_probe.c
> index dfcd28a8..5cf5fe27 100644
> --- a/libdtrace/dt_probe.c
> +++ b/libdtrace/dt_probe.c
> @@ -951,6 +951,7 @@ dt_probe_args_info(dtrace_hdl_t *dtp, dt_probe_t *prp)
>   
>   		prp->mapping[i] = argv[i].mapping;
>   		prp->argv[i] = dtt;
> +		prp->xargv[i]->dn_flags |= argv[i].flags;
>   	}
>   
>   	dt_free(dtp, argv);
> diff --git a/libdtrace/dt_prov_proc.c b/libdtrace/dt_prov_proc.c
> index 57753b94..4544f594 100644
> --- a/libdtrace/dt_prov_proc.c
> +++ b/libdtrace/dt_prov_proc.c
> @@ -82,32 +82,32 @@ typedef struct probe_arg {
>    * that provides no arguments.
>    */
>   static probe_arg_t probe_args[] = {
> -	{ "create", 0, { 0, "struct task_struct *", "psinfo_t *" } },
> -	{ "exec", 0, { 0, "string", } },
> -	{ "exec-failure", 0, { 0, "int", } },
> +	{ "create", 0, { 0, 0, "struct task_struct *", "psinfo_t *" } },
> +	{ "exec", 0, { 0, DT_NF_USERLAND, "string", } },
> +	{ "exec-failure", 0, { 0, 0, "int", } },
>   	{ "exec-success", },
> -	{ "exit", 0, { 0, "int", } },
> +	{ "exit", 0, { 0, 0, "int", } },
>   #if 0
> -	{ "fault", 0, { 0, "int", } },
> -	{ "fault", 1, { 1, "siginfo_t *", } },
> +	{ "fault", 0, { 0, 0, "int", } },
> +	{ "fault", 1, { 1, 0, "siginfo_t *", } },
>   #endif
> -	{ "lwp-create", 0, { 0, "struct task_struct *", "lwpsinfo_t *" } },
> -	{ "lwp-create", 1, { 0, "struct task_struct *", "psinfo_t *" } },
> +	{ "lwp-create", 0, { 0, 0, "struct task_struct *", "lwpsinfo_t *" } },
> +	{ "lwp-create", 1, { 0, 0, "struct task_struct *", "psinfo_t *" } },
>   	{ "lwp-exit", },
>   	{ "lwp-start", },
>   #if 0
> -	{ "signal-clear", 0, { 0, "int", } },
> -	{ "signal-clear", 1, { 1, "siginfo_t *", } },
> +	{ "signal-clear", 0, { 0, 0, "int", } },
> +	{ "signal-clear", 1, { 1, 0, "siginfo_t *", } },

I haven't been following proc, but signal-clear is documented as having 
only one arg?

>   #endif
> -	{ "signal-discard", 0, { 0, "struct task_struct *", "lwpsinfo_t *" } },
> -	{ "signal-discard", 1, { 0, "struct task_struct *", "psinfo_t *" } },
> -	{ "signal-discard", 2, { 1, "int", } },
> -	{ "signal-handle", 0, { 0, "int", } },
> -	{ "signal-handle", 1, { 1, "siginfo_t *", } },
> -	{ "signal-handle", 2, { 2, "void (*)(void)", } },
> -	{ "signal-send", 0, { 0, "struct task_struct *", "lwpsinfo_t *" } },
> -	{ "signal-send", 1, { 0, "struct task_struct *", "psinfo_t *" } },
> -	{ "signal-send", 2, { 1, "int", } },
> +	{ "signal-discard", 0, { 0, 0, "struct task_struct *", "lwpsinfo_t *" } },
> +	{ "signal-discard", 1, { 0, 0, "struct task_struct *", "psinfo_t *" } },
> +	{ "signal-discard", 2, { 1, 0, "int", } },
> +	{ "signal-handle", 0, { 0, 0, "int", } },
> +	{ "signal-handle", 1, { 1, 0, "siginfo_t *", } },
> +	{ "signal-handle", 2, { 2, 0, "void (*)(void)", } },
> +	{ "signal-send", 0, { 0, 0, "struct task_struct *", "lwpsinfo_t *" } },
> +	{ "signal-send", 1, { 0, 0, "struct task_struct *", "psinfo_t *" } },
> +	{ "signal-send", 2, { 1, 0, "int", } },
>   	{ "start", },
>   };
>   
>