[DTrace-devel] [PATCH] cg: ensure string results in a ternary are padded to strsize

[Eugene Loh]

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

That said:

*)  Is it possible to construct a test?

*)  Has there been an audit to ferret out other instances of this 
problem?  Also, I'm confused what position we're taking here. We're 
saying that it's possible for a string (which in D has strsize) to have 
garbage after the NUL byte, but we'll deal with this situation only in 
the case that it's an argument of a ternary operation?  Why do we not 
say instead that a strsize string will not have garbage after the NUL 
byte, so that a strsize copy of a string will be safe (regardless of 
whether it's a ternary op)?

*)  What position are we taking here on BPF register pressure? E.g., 
what if dst were %r0 (okay, let's hope not) or even just %r1-%r5 (even 
just %r4 or %r5)?  One position is that we don't do a robust job of BPF 
reg management anyhow and someday we're going to have to clean that up 
and so being sloppy now is okay or even necessary.  Another position is 
that we can add a little code to improve things (even if other areas of 
dt_cg.c vary considerably in quality).  E.g., one can fill-spill regs 
between the two function calls, costing extra BPF instructions only if 
needed. (That's already a pretty common practice in dt_cg.c.)  For extra 
robustness (beyond what we currently do in dt_cg.c), one can move 
%r1=dst and %r3=src in whatever order makes sense (more complicated and 
less likely to be useful).  Anyhow, fill-spill regs between function 
calls is a pretty common practice in dt_cg.c and I would think would 
make to do here.

On 2/7/25 01:11, Kris Van Hees wrote:
> The result of a string ternary was copied from either left or right
> value using a memcpy() of strsize, but that could result in garbage
> being included in the result beyond the terminating 0-byte of the
> string value.  If the result was e.g. assigned to a char-array
> translator member, that garbage could affect consumer output.
>
> The ternary will now copy the string using dt_cg_strcpy() which ensures
> that any string < strsize will be padded with 0-bytes up to strsize.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_cg.c | 68 ++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 52 insertions(+), 16 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index a5c9aa09..b48e27f0 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1371,6 +1371,19 @@ dt_cg_fill_gap(dt_pcb_t *pcb, int gap)
>   		emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_9, off, 0));
>   }
>   
> +/*
> + * Store a pointer to the 'memory block of zeros' in reg.
> + */
> +static void
> +dt_cg_zerosptr(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
> +{
> +	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
> +	dt_ident_t	*zero_off = dt_dlib_get_var(dtp, "ZERO_OFF");
> +
> +	dt_cg_access_dctx(reg, dlp, drp, DCTX_STRTAB);
> +	emite(dlp, BPF_ALU64_IMM(BPF_ADD, reg, -1), zero_off);
> +}
> +
>   static void
>   dt_cg_memcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src, size_t size)
>   {
> @@ -1394,6 +1407,43 @@ dt_cg_memcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src, size_t size)
>   	dt_regset_free(drp, BPF_REG_0);
>   }
>   
> +/*
> + * Copy a string from the pointer stored in the src register to the pointer
> + * stored in the dst register.  At most strsize characters will be copied, and
> + * if the source string is less than strsize characters, the remainder will be
> + * filled with 0s.
> + */
> +static void
> +dt_cg_strcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src)
> +{
> +	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
> +	uint_t		lbl_ok = dt_irlist_label(dlp);
> +	size_t		size = dtp->dt_options[DTRACEOPT_STRSIZE];
> +
> +	if (dt_regset_xalloc_args(drp) == -1)
> +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> +	emit(dlp,  BPF_MOV_REG(BPF_REG_1, dst));
> +	emit(dlp,  BPF_MOV_IMM(BPF_REG_2, size));
> +	emit(dlp,  BPF_MOV_REG(BPF_REG_3, src));
> +	dt_regset_xalloc(drp, BPF_REG_0);
> +	emit(dlp,  BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel_str]));
> +
> +	emit(dlp,  BPF_BRANCH_IMM(BPF_JSGE, BPF_REG_0, 0, lbl_ok));
> +	dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, DT_ISREG, src);
> +
> +	emitl(dlp, lbl_ok,
> +		   BPF_NOP());
> +	emit(dlp,  BPF_MOV_REG(BPF_REG_1, dst));
> +	emit(dlp,  BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_0));
> +	emit(dlp,  BPF_MOV_IMM(BPF_REG_2, size));
> +	emit(dlp,  BPF_ALU64_REG(BPF_SUB, BPF_REG_2, BPF_REG_0));
> +	dt_cg_zerosptr(BPF_REG_3, dlp, drp);
> +	emit(dlp,  BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
> +	dt_regset_free(drp, BPF_REG_0);
> +	dt_regset_free_args(drp);
> +}
> +
>   static void
>   dt_cg_spill_store(int reg)
>   {
> @@ -3040,19 +3090,6 @@ dt_cg_pop_stack(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
>   	dt_regset_free(drp, treg);
>   }
>   
> -/*
> - * Store a pointer to the 'memory block of zeros' in reg.
> - */
> -static void
> -dt_cg_zerosptr(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
> -{
> -	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
> -	dt_ident_t	*zero_off = dt_dlib_get_var(dtp, "ZERO_OFF");
> -
> -	dt_cg_access_dctx(reg, dlp, drp, DCTX_STRTAB);
> -	emite(dlp, BPF_ALU64_IMM(BPF_ADD, reg, -1), zero_off);
> -}
> -
>   /*
>    * Generate code to promote signed scalars (size < 64 bits) to native register
>    * size (64 bits).
> @@ -4603,7 +4640,7 @@ dt_cg_ternary_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   	 * dn_right).
>   	 */
>   	if (dt_node_is_string(dnp)) {
> -		uint_t lbl_null = dt_irlist_label(dlp);
> +		uint_t	lbl_null = dt_irlist_label(dlp);
>   
>   		emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, dnp->dn_reg, 0, lbl_null));
>   
> @@ -4619,8 +4656,7 @@ dt_cg_ternary_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   		dt_cg_access_dctx(dnp->dn_reg, dlp, drp, DCTX_MEM);
>   		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, dnp->dn_tstring->dn_value));
>   
> -		dt_cg_memcpy(dlp, drp, dnp->dn_reg, BPF_REG_0,
> -			     yypcb->pcb_hdl->dt_options[DTRACEOPT_STRSIZE]);
> +		dt_cg_strcpy(dlp, drp, dnp->dn_reg, BPF_REG_0);
>   
>   		emitl(dlp, lbl_null,
>   			   BPF_NOP());