[DTrace-devel] [PATCH] cg: fix store/load for struct and union members for alloca pointers

[Eugene Loh]

I'm not sure I follow alloca-taint logic well enough to review this.  
Maybe Nick can take a peek?

Also, did you say you had tests for this?  They should be included in 
the patch.  Once the inet_ntop patch has been added (after this patch), 
there will be a test, but more basic and thorough test coverage as a 
part of the current patch would be even better. Particularly since more 
than one code path is being modified.

And a few other things...

On 8/30/23 22:54, Kris Van Hees via DTrace-devel wrote:
> Storing to and loading from struct or unnion members of alloca pointers

union

> was not properly turning the alloca pointer into a real pointer, nor was
> it performing boundary checks.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_cg.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 40df8d65..95dd57ad 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3051,7 +3051,7 @@ dt_cg_store(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dst)
>   	int		dreg = dst->dn_reg;
>   
>   	/*
> -	 * If we're loading a bit-field, the size of our store is found by
> +	 * If we're storing into a bit-field, the size of our store is found by
>   	 * rounding dst's cte_bits up to a byte boundary and then finding the
>   	 * nearest power of two to this value (see clp2(), above).
>   	 */
> @@ -3062,13 +3062,18 @@ dt_cg_store(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dst)
>   		size = dt_node_type_size(dst);
>   
>   	/*
> -	 * If we're loading a writable non-alloca lvalue, and it's a
> +	 * If we're storing into a writable non-alloca lvalue, and it's a

non-alloca?  IIUC, we do not check that.  So, the "non-alloca" comment 
does not agree with what the code is doing?

In general, this comment doesn't make things any clearer than what the 
code says and so does not have much value;  it can be pruned down.

And I'm not sure of the logic... this is perhaps where Nick can chime in.

>   	 * dereference, and *its* child is an alloca pointer, then this is a
>   	 * dereferenced alloca pointer and needs bounds-checking (which could
>   	 * not be done at deref time due to not knowing the size of the write).
> +	 *
> +	 * If we're storing into a writable alloca lvalue, we know it needs
> +	 * bounds-checking as well.
>   	 */
> -	if (dst->dn_flags & DT_NF_WRITABLE && dst->dn_flags & DT_NF_LVALUE
> -	    && dst->dn_op == DT_TOK_DEREF && dst->dn_child->dn_flags & DT_NF_ALLOCA) {
> +	if (dst->dn_flags & DT_NF_WRITABLE && dst->dn_flags & DT_NF_LVALUE &&
> +	    ((dst->dn_op == DT_TOK_DEREF &&
> +	      dst->dn_child->dn_flags & DT_NF_ALLOCA) ||
> +	     dst->dn_flags & DT_NF_ALLOCA)) {
>   		assert(!(dst->dn_flags & DT_NF_BITFIELD));
>   
>   		if ((dreg = dt_regset_alloc(drp)) == -1)
> @@ -6480,8 +6485,20 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   
>   			op = dt_cg_ldsize(dnp, ctfp, m.ctm_type, &size);
>   
> +			/*
> +			 * If the left-hand side of PTR or DOT is an alloca
> +			 * pointer, bounds-check the pointer reference.
> +			 */
> +			if (dnp->dn_left->dn_flags & DT_NF_ALLOCA) {
> +				assert(dnp->dn_flags & DT_NF_ALLOCA);
> +				dt_cg_alloca_access_check(dlp, drp, dnp->dn_reg,
> +							  DT_ISIMM, size);
> +				dt_cg_alloca_ptr(dlp, drp, dnp->dn_reg,
> +						 dnp->dn_reg);
> +			}
> +
>   			if (dnp->dn_left->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))
> -				emit(dlp, BPF_LOAD(op, dnp->dn_left->dn_reg, dnp->dn_left->dn_reg, 0));
> +				emit(dlp, BPF_LOAD(op, dnp->dn_reg, dnp->dn_reg, 0));

Okay on s/dn_left->//, but then there is another instance where this 
change should be made as well, right above this code at "if 
(m.ctm_offset != 0)".

>   			else
>   				dt_cg_load_scalar(dnp->dn_left, op, size, dlp, drp);
>   		}