[DTrace-devel] [PATCH 03/12] Fix alloca() non-constant size check

[Kris Van Hees]

On Fri, Jul 22, 2022 at 10:49:25AM -0700, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> 
> Good enough, I guess, albeit with the usual number of questions.
> 
> First, I am not real familiar with the alloca()/scratch stuff.  So maybe
> Nick wants to take a look.
> 
> Specifically, I do not understand the whole "8" thing.  (I guess there is an
> initial 8-byte padding to represent a NULL pointer. This padding is so
> invasive, hard-coded into every access to the scratch area.  Might it have
> been better placed at the *end* of the scratch area?  I don't know.  But,
> again, this eccentricity is being exposed in so many places in the code...
> that magic "8" value is all over the place.)
> 
> I don't get why DTRACEOPT_SCRATCHSIZE is not the value of the scratchsize
> option, as the name would suggest (at least to me).

The initial 8 bytes are there because they occur at offset 0 which means that
an alloca offset of 0 maps to a NULL pointer quite nicely.

> Having that "8" included in DTRACEOPT_SCRATCHSIZE also makes me wonder
> if these checks are right:
> 
>     dt_bpf.c:
>         if (dtp->dt_options[DTRACEOPT_SCRATCHSIZE] > 0)
>             create_gmap(dtp, "scratchmem", ...);
> 
>     dt_cg.c:
>         if (dtp->dt_options[DTRACEOPT_SCRATCHSIZE] > 0)
>                 DT_CG_STORE_MAP_PTR("scratchmem", DCTX_SCRATCHMEM);

The 8 extra bytes are only included when scratchsize is not 0, i.e. when we
are actually making scratch space available.

> As for this specific patch:
> 
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> @@ -4177,8 +4177,8 @@ dt_cg_subr_alloca(dt_node_t *dnp, dt_irlist_t *dlp,
> dt_regset_t *drp)
>      emit(dlp,  BPF_ALU64_IMM(BPF_ADD, next, 7));
>      emit(dlp,  BPF_ALU64_IMM(BPF_AND, next, (int) -8));
> 
> -    emit(dlp,  BPF_BRANCH_IMM(BPF_JGT, next, opt_scratchsize + 8,
> lbl_err));
> +    emit(dlp,  BPF_BRANCH_IMM(BPF_JGT, next, opt_scratchsize, lbl_err));
> -    emit(dlp,  BPF_BRANCH_IMM(BPF_JLE, dnp->dn_reg, opt_scratchsize + 8,
> +    emit(dlp,  BPF_BRANCH_IMM(BPF_JLE, dnp->dn_reg, opt_scratchsize - 8,
>                    lbl_ok));
>      emitl(dlp, lbl_err,
>             BPF_NOP());
> 
> I can guess why we check dnp->dn_reg, but wouldn't mind an explanation. 
> E.g., I could also believe that if we have checked that next is in-bounds,
> then certainly dnp->dn_reg must also be, making that check unnecessary.  (I
> could argue to myself that there is an edge case, but it's probably better
> for me to ask for explanation.)

As you know, the verifier does not track dependencies between register values,
so when you e.g. have mov %r1, %r2 and you bounds check %r1, the bounds for %r2
are not updated (and vice versa).  This is a similar case where dnp->dn_reg is
related to next, but one bounds check is not sufficient to satisfy the
verifier.
> 
> @@ -4199,7 +4199,7 @@ dt_cg_subr_bcopy(dt_node_t *dnp, dt_irlist_t *dlp,
> dt_regset_t *drp)
>      dt_node_t    *src = dnp->dn_args;
>      dt_node_t    *dst = src->dn_list;
>      dt_node_t    *size = dst->dn_list;
> -    int        maxsize = yypcb->pcb_hdl->dt_options[DTRACEOPT_SCRATCHSIZE];
> +    int        maxsize = yypcb->pcb_hdl->dt_options[DTRACEOPT_SCRATCHSIZE]
> - 8;
>      uint_t        lbl_badsize = dt_irlist_label(dlp);
>      uint_t        lbl_ok = dt_irlist_label(dlp);
> 
> Why is there a maxsize at all?  Put another way, why is it used to check
> size?  Why doesn't the dt_cg_alloca_access_check() that follows the check
> suffice?

You'd have to go back to the original patch Nick wrote and submitted for bcopy
and see the discussion there.  In short, size is used in the access check, and
therefore needs to be bounds checked itself or the verifier will complain.

	Kris