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

[Kris Van Hees]

On Thu, Aug 31, 2023 at 12:20:49PM -0400, Eugene Loh via DTrace-devel wrote:
> I'm not sure I follow alloca-taint logic well enough to review this.  Maybe
> Nick can take a peek?

This actually does not have anything to do with the taint :)

> 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.

Well, yes.. and I don't know why they are not in this one...  Let me post v2
after carefully reviewing that they are actually part of the patch.

> 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);
> >   		}
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel