[DTrace-devel] [PATCH] cg: lift restriction of 1st arg to bcopy() not being alloca
Kris Van Hees
kris.van.hees at oracle.com
Fri Jan 6 22:44:37 UTC 2023
On Fri, Jan 06, 2023 at 03:25:39PM -0500, Kris Van Hees via DTrace-devel wrote:
> On Fri, Jan 06, 2023 at 02:29:09PM -0500, Eugene Loh via DTrace-devel wrote:
> > Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> > with comments below...
> >
> > To start:
> >
> > *) Does this patch modify behavior of copyinto() as well? If so, shouldn't
> > that be mentioned and tested?
>
> Yes and no. But nothing that needs documenting and testing. The copyinto()
> subroutine copies from userspace addresses so those are never alloca pointers.
> But no dtrace version has ever validated the source pointer for copyinto()
> anyway because it truly can be an arbitrary pointer.
>
> > *) Modified files (including dt_cg.c) should have 2023 copyright.
>
> Ah yes, forgot this was work from 2022 that I never pushed for review until
> now.
>
> > On 1/4/23 01:32, Kris Van Hees via DTrace-devel wrote:
> > > The documentation states that the source address is not allowed to be
> > > within the scratch memory region, but the legacy implementation does
> > > not actually enforce that restriction, nor does it seem necessary.
> >
> > I've lost track, but have we standardized on what "legacy" means in this
> > context? Will future developers know what this means? Also, isn't Solaris
> > the more compelling precedent?
>
> We use it to refer to both the Solaris version and the pre-BPF versiobn, in
> that they almost always implement the same semantics. But I can update the
> commit message to explicitly mention that dtrace has never enforced that
> restriction.
>
> > > This patch also moves bcopy tests into their own directory.
> > >
> > > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > >
> > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > @@ -4411,10 +4411,7 @@ dt_cg_subr_bcopy_impl(dt_node_t *dnp, dt_node_t *dst, dt_node_t *src,
> > > "\t argument: non-alloca pointer\n",
> > > is_bcopy ? "bcopy" : "copyinto", is_bcopy ? 2 : 3);
> > > /* The dst will be NULL-checked in the alloca access check below. */
> > > -
> > > - dt_cg_alloca_access_check(dlp, drp, dst->dn_reg,
> > > - DT_ISREG, size->dn_reg);
> > > - dt_cg_alloca_ptr(dlp, drp, dst->dn_reg, dst->dn_reg);
> > > + dt_cg_check_ptr_arg(dlp, drp, dst, size);
> > > if (dt_regset_xalloc_args(drp) == -1)
> > > longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> >
> > Should that comment also be removed, since the alloca_access_check() call is
> > removed?
>
> No, but I will update it to make it clear that NULL and access checking is
> doing in dt_cg_check_ptr_arg().
Actually, yes, I will remove it. We already have an earlier comment that we
are validating the destination.
> > > diff --git a/test/unittest/funcs/bcopy/err.badbcopy7.d b/test/unittest/funcs/bcopy/err.badbcopy7.d
> > > new file mode 100644
> > > index 00000000..c8fffea5
> > > --- /dev/null
> > > +++ b/test/unittest/funcs/bcopy/err.badbcopy7.d
> > > @@ -0,0 +1,29 @@
> > > +/*
> > > + * Oracle Linux DTrace.
> > > + * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
> >
> > 2023? And drop the 2006. Whatever file this was cut and pasted from surely
> > didn't contribute much to this test. After all, as far as git is concerned,
> > this is a new test.
>
> Good point.
>
> > > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > > + * http://oss.oracle.com/licenses/upl.
> > > + */
> > > +
> > > +/*
> > > + * ASSERTION: bcopy from an alloca pointer is subject to size checks
> > > + *
> > > + * SECTION: Actions and Subroutines/alloca();
> > > + * Actions and Subroutines/bcopy()
> > > + *
> >
> > Stylistic consistency would probably drop that blank comment line.
>
> Hm, yes. Though we have many tests that are still inconsistent - but doesn't
> hurt to do it here since I believe we have been doing that for newer tests (or
> updated tests).
>
> > > + */
> > > +
> > > +#pragma D option quiet
> > > +
> > > +BEGIN
> > > +{
> > > + d = alloca(20);
> > > + s = alloca(10);
> > > + bcopy(s, d, 20);
> >
> > Not a big deal, but s and d might as well be src and dst.
> >
> > > + exit(0);
> > > +}
> > > +
> > > +ERROR
> > > +{
> > > + exit(1);
> > > +}
> > > diff --git a/test/unittest/funcs/err.badbcopy8.d b/test/unittest/funcs/bcopy/err.badbcopy8.d
> > > similarity index 100%
> > > rename from test/unittest/funcs/err.badbcopy8.d
> > > rename to test/unittest/funcs/bcopy/err.badbcopy8.d
> >
> > Okay, so err.badbcopy8.d is being used "as is," but shouldn't it get some
> > scrutiny? It has:
> > ptr = alloca(20);
> > /* Attempt to bcopy to scratch memory that isn't allocated,
> > with a max exceeding the verifier-checked bound of
> > 2*scratchsize. */
> > bcopy((void *)&`max_pfn, ptr, 2048000);
> > But the scratch memory clearly *IS* allocated. If the point is the size is
> > too large, then compared to what? Apparently, larger than the allocated 20
> > is just fine. Maybe even up to scratchsize is fine. Over 1*scratchsize is
> > not okay, despite the comment. And testing with 2048000 seems just plain
> > irrelevant.
>
> I am simply moving the bcopy tests to their own subdirectory. I am not
> validating the tests being moved within the context of this patch. I would
> suggest checking with Nick on whether this test is valid as-is or whether it
> is not (or to provide a more clear explanation about it).
>
> > > diff --git a/test/unittest/funcs/err.badbcopy7.d b/test/unittest/funcs/bcopy/tst.bcopy2.d
> > > similarity index 65%
> > > rename from test/unittest/funcs/err.badbcopy7.d
> > > rename to test/unittest/funcs/bcopy/tst.bcopy2.d
> > > index d7390122..6fa01d1c 100644
> > > --- a/test/unittest/funcs/err.badbcopy7.d
> > > +++ b/test/unittest/funcs/bcopy/tst.bcopy2.d
> > > @@ -6,8 +6,8 @@
> > > */
> > > /*
> > > - * ASSERTION:
> > > - * bcopy should not copy when the source is scratch space
> > > + * ASSERTION: bcopy to scratch space is allowed (even though documentation
> > > + * claims that it is not)
> > > *
> > > * SECTION: Actions and Subroutines/alloca();
> > > * Actions and Subroutines/bcopy()
> > > @@ -16,14 +16,13 @@
> > > #pragma D option quiet
> > > -
> > > BEGIN
> > > {
> > > - ptr = alloca(sizeof(unsigned long));
> > > + ptr = (unsigned long *)alloca(sizeof(unsigned long));
> > > bcopy((void *)&`max_pfn, ptr, sizeof(unsigned long));
> > > - ptr2 = alloca(sizeof(unsigned long));
> > > + ptr2 = (unsigned long *)alloca(sizeof(unsigned long));
> > > bcopy(ptr, ptr2, sizeof(unsigned long));
> > > - exit(0);
> > > + exit(*ptr == *ptr2 ? 0 : 1);
> > > }
> > > ERROR
> >
> > I realize that this test is inherited from past work, but using
> > bcopy(&`max_pfn,...) to initialize *ptr unnecessarily complicates this
> > rather simple test. Why not just *ptr=0x12345678, or something? That makes
> > clearer what's going on. It'd also be clearer to use "src" and "dst" rather
> > than "ptr" and "ptr2".
>
> Overall, I agree, but I wanted to keep the original negative test as much as
> possible to indicate that whereas the original test was verifying that using
> an alloca pointer as source was not allowed, the new test verifies that it is
> in fact allowed (and that it works).
>
> Kris
>
> _______________________________________________
> 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