[DTrace-devel] [PATCH] Add run-time checks on bounds for scalar-array access
Kris Van Hees
kris.van.hees at oracle.com
Fri Feb 24 16:40:13 UTC 2023
On Fri, Feb 24, 2023 at 01:41:55AM -0500, Kris Van Hees via DTrace-devel wrote:
> On Wed, Feb 08, 2023 at 01:55:29AM -0500, eugene.loh--- via DTrace-devel wrote:
> > From: Eugene Loh <eugene.loh at oracle.com>
> >
> > If a scalar (nonassociated, linearly indexed) array is accessed with
> > a constant index, the parser checks that the access is in-bounds.
> > With other index types, however, the value is not known until runtime.
> > Since the index value is not being checked at runtime, the BPF verifier
> > does not allow the code to be loaded, lest an invalid address is
> > dereferenced.
> >
> > Add a runtime check when we try to add an offset to a scalar array
> > address.
> >
> > Orabug: 35045463
> > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > ---
> > libdtrace/dt_cg.c | 39 +++++++++++++++++++
> > test/unittest/arrays/tst.declared-bounds.d | 1 -
> > test/unittest/arrays/tst.declared-bounds.r | 2 +-
> > .../arrays/tst.declared-bounds.runtime_in.d | 31 +++++++++++++++
> > .../arrays/tst.declared-bounds.runtime_in.r | 1 +
> > .../arrays/tst.declared-bounds.runtime_out.d | 31 +++++++++++++++
> > .../arrays/tst.declared-bounds.runtime_out.r | 3 ++
> > 7 files changed, 106 insertions(+), 2 deletions(-)
> > create mode 100644 test/unittest/arrays/tst.declared-bounds.runtime_in.d
> > create mode 100644 test/unittest/arrays/tst.declared-bounds.runtime_in.r
> > create mode 100644 test/unittest/arrays/tst.declared-bounds.runtime_out.d
> > create mode 100644 test/unittest/arrays/tst.declared-bounds.runtime_out.r
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index c2eb85c0..50846942 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -3212,6 +3212,9 @@ dt_cg_arithmetic_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> > int lp_is_ptr = dt_node_is_pointer(dnp->dn_left);
> > int rp_is_ptr = dt_node_is_pointer(dnp->dn_right);
> >
> > + ctf_file_t *ctfp = dnp->dn_left->dn_ctfp;
> > + ctf_id_t type = ctf_type_resolve(ctfp, dnp->dn_left->dn_type);
> > +
> > if (lp_is_ptr && rp_is_ptr) {
> > assert(dnp->dn_op == DT_TOK_SUB);
> > is_ptr_op = 0;
> > @@ -3283,6 +3286,42 @@ dt_cg_arithmetic_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> >
> > emitl(dlp, lbl_L5,
> > BPF_NOP());
> > + } else if (op == BPF_ADD
> > + && dnp->dn_right->dn_kind != DT_NODE_INT
> > + && ctf_type_kind(ctfp, type) == CTF_K_ARRAY) {
> > + /*
> > + * If the left-hand argument is a CTF_K_ARRAY and we are adding
> > + * an offset -- which is exactly what the parser will try to do
> > + * if an element of a scalar array is being accessed -- then the
> > + * BPF verifier will complain if it knows nothing about the offset.
> > + * Meanwhile, if the offset is an DT_NODE_INT, then we have already
> > + * checked it is in bounds. So, add a run-time check only when
> > + * the right-hand argument is not DT_NODE_INT.
> > + */
> > + ctf_arinfo_t r;
> > + uint_t L1 = dt_irlist_label(dlp);
> > + ssize_t elem_size;
> > +
> > + if (ctf_array_info(ctfp, type, &r) != 0) {
> > + yypcb->pcb_hdl->dt_ctferr = ctf_errno(ctfp);
> > + longjmp(yypcb->pcb_jmpbuf, EDT_CTF);
> > + }
> > +
> > + elem_size = ctf_type_size(ctfp, r.ctr_contents);
> > +
> > + emit(dlp, BPF_BRANCH_IMM(BPF_JLT, dnp->dn_right->dn_reg, r.ctr_nelems * elem_size, L1));
> > +
> > + /*
> > + * In case of an error, report the index rather than the address, because:
> > + * * the address probably means nothing to the user
> > + * * we would have to do some trickery to fool the
> > + * BPF verifier to let us compute the address.
> > + */
> > + emit(dlp, BPF_ALU64_IMM(BPF_DIV, dnp->dn_right->dn_reg, elem_size));
> > + dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, DT_ISREG, dnp->dn_right->dn_reg);
>
> I don't think this really makes sense. If we are raising a BADADDR fault then
> the value is supposed to be an address. Reporting something different would
> be confusing and more indicative of the array pointer being NULL which it is
> definitely not supposed to be. Why not (in case of error) use the scalarizer
> approach on the address and adjust it using the offset. In other words, use
> the scalarizer mechanism on dnp->dn_left->dn_reg, and then use:
>
> BPF_ALU64_REG(op, dnp->dn_left->dn_reg, dnp->dn_right->dn_reg));
>
> to calculate the address to report the fault for.
I changed my mind. You are correct that reporting the index is the most useful
information. My main issue was that it would be wrong to report it as a
BADADDR fault. So... I introduced a new fault (BADINDEX) that should be used
here.
We should look at other places where we report BADADDR and see if they really
should be reported as an 'index out of bounds' fault. Indexing a tring past
its boundaries would come to mind as a good place to use that kind of fault.
Anyway, that is a TODO.
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
... with this tiny change
> > +
> > + emitl(dlp, L1,
> > + BPF_ALU64_REG(op, dnp->dn_left->dn_reg, dnp->dn_right->dn_reg));
> > } else
> > emit(dlp, BPF_ALU64_REG(op, dnp->dn_left->dn_reg, dnp->dn_right->dn_reg));
> >
> > diff --git a/test/unittest/arrays/tst.declared-bounds.d b/test/unittest/arrays/tst.declared-bounds.d
> > index ff7f3527..b11b5cb7 100644
> > --- a/test/unittest/arrays/tst.declared-bounds.d
> > +++ b/test/unittest/arrays/tst.declared-bounds.d
> > @@ -4,7 +4,6 @@
> > * Licensed under the Universal Permissive License v 1.0 as shown at
> > * http://oss.oracle.com/licenses/upl.
> > */
> > -/* @@xfail: dtv2 */
> >
> > /*
> > * ASSERTION:
> > diff --git a/test/unittest/arrays/tst.declared-bounds.r b/test/unittest/arrays/tst.declared-bounds.r
> > index 11573a18..b604cc0a 100644
> > --- a/test/unittest/arrays/tst.declared-bounds.r
> > +++ b/test/unittest/arrays/tst.declared-bounds.r
> > @@ -1,5 +1,5 @@
> > FUNCTION:NAME
> > - :BEGIN 0
> > + :BEGIN 0
> >
> > -- @@stderr --
> > dtrace: script 'test/unittest/arrays/tst.declared-bounds.d' matched 1 probe
> > diff --git a/test/unittest/arrays/tst.declared-bounds.runtime_in.d b/test/unittest/arrays/tst.declared-bounds.runtime_in.d
> > new file mode 100644
> > index 00000000..8e36e428
> > --- /dev/null
> > +++ b/test/unittest/arrays/tst.declared-bounds.runtime_in.d
> > @@ -0,0 +1,31 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +/*
> > + * ASSERTION: Within-bounds array assignments work even when index
> > + * requires run-time check.
> > + *
> > + * SECTION: Pointers and Arrays/Array Declarations and Storage
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +int a[5];
> > +
> > +BEGIN
> > +{
> > + i = 0;
> > + a[i] = 12345678;
> > + trace(a[i]);
> > + exit(0);
> > +}
> > +
> > +ERROR
> > +{
> > + trace("run-time error");
> > + exit(1);
> > +}
> > diff --git a/test/unittest/arrays/tst.declared-bounds.runtime_in.r b/test/unittest/arrays/tst.declared-bounds.runtime_in.r
> > new file mode 100644
> > index 00000000..97b5955f
> > --- /dev/null
> > +++ b/test/unittest/arrays/tst.declared-bounds.runtime_in.r
> > @@ -0,0 +1 @@
> > +12345678
> > diff --git a/test/unittest/arrays/tst.declared-bounds.runtime_out.d b/test/unittest/arrays/tst.declared-bounds.runtime_out.d
> > new file mode 100644
> > index 00000000..95397491
> > --- /dev/null
> > +++ b/test/unittest/arrays/tst.declared-bounds.runtime_out.d
> > @@ -0,0 +1,31 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +/*
> > + * ASSERTION: Out-of-bounds array assignments work even when index
> > + * requires run-time check.
> > + *
> > + * SECTION: Pointers and Arrays/Array Declarations and Storage
> > + */
> > +
> > +#pragma D option quiet
> > +
> > +int a[5];
> > +
> > +BEGIN
> > +{
> > + i = 8;
> > + a[i] = 12345678;
> > + trace(a[i]);
> > + exit(1);
> > +}
> > +
> > +ERROR
> > +{
> > + trace("expected run-time error");
> > + exit(0);
> > +}
> > diff --git a/test/unittest/arrays/tst.declared-bounds.runtime_out.r b/test/unittest/arrays/tst.declared-bounds.runtime_out.r
> > new file mode 100644
> > index 00000000..5b4d3eb5
> > --- /dev/null
> > +++ b/test/unittest/arrays/tst.declared-bounds.runtime_out.r
> > @@ -0,0 +1,3 @@
> > +expected run-time error
> > +-- @@stderr --
> > +dtrace: error on enabled probe ID 3 (ID 1: dtrace:::BEGIN): invalid address ({ptr}) in action #1 at BPF pc NNN
> > --
> > 2.18.4
> >
> >
> > _______________________________________________
> > DTrace-devel mailing list
> > DTrace-devel at oss.oracle.com
> > https://oss.oracle.com/mailman/listinfo/dtrace-devel
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list